|
From: Julian S. <js...@ac...> - 2012-02-12 21:13:04
|
Great stuff, especially the numbers. > Putting an upper bound on the number of nodes we allow sameIREXprs > to recurse into makes sense. But since proving equality was key > to fixing #287260 and recursing is not the norm I suggest to use > a large limit. The patch sets it to 100. Having a natural aversion to global state, I'd have maybe preferred a depth limit, but no matter. If we ever come to multithread the JIT we'll have way bigger problems to deal with. Re the 100, this is my only concern. I think that we could still get hit pretty hard in the worst case. Considering that your numbers show a max value of 25 (memcheck, ffbench) and that the perf suite covers about 20000 lines of code, I'd prefer to use that value (25) or something marginally above (30) on the basis that that will cover 99.9...% of uses that will occur. AFAICS this wouldn't limit us much whilst providing more safety than 100. There are similar worst-case limiters in other parts of iropt, in the tree-builder for one. > I left the code for gathering those stats in the patch. Might become > useful again. It can be enabled by setting STATS_IROPT to 1. Fine. +1 to land if you're happy with the threshold change. J |
|
From: Florian K. <br...@ac...> - 2012-02-13 00:25:38
|
On 02/12/2012 04:05 PM, Julian Seward wrote: > > Great stuff, especially the numbers. > >> Putting an upper bound on the number of nodes we allow sameIREXprs >> to recurse into makes sense. But since proving equality was key >> to fixing #287260 and recursing is not the norm I suggest to use >> a large limit. The patch sets it to 100. > > Having a natural aversion to global state, I'd have maybe preferred > a depth limit, but no matter. If we ever come to multithread the JIT > we'll have way bigger problems to deal with. > The depth is not as good a measure as is the number of nodes. A binary tree of depth 4 can have anywhere between 4 nodes and 15 nodes. That being said, the global state could still be avoided. But I left it as is for now. > Re the 100, this is my only concern. I think that we could still get > hit pretty hard in the worst case. > I changed it down to 30 and committed it. The numbers I had reported are deceiving. Although the number of traversed nodes was <= 25 in all cases it can be much higher. I just ran some proprietary application which showed max_nodes = 62 !! Which only confirms what we already know: improving the perf bucket with a larger variety of real apps would be a very good thing... Florian |