The patch to be attached has a changelog that says this: This commit refactors the parser to more uniformly propagate flag bits down and up the parse process, as the parser descends and returns into nested blocks. Some flags get passed town to subscopes, some apply to specific scopes only, and some get unioned up after parsing subscopes. The goal is to eventually be very precise with scoping information, once we have block scopes: one block scope might use `eval', which would require the emission of a symbol table within that block and containing blocks, whereas another block in the same function might not, allowing us to not emit a symbol table. *** I have to apologize for this being another one of those "refactoring" commits. I'm getting tired of them too :) But, I do think this is a positive change to make that will make some of the block scoping stuff easier.
Created attachment 127976 [details] Patch
Oliver, what's the best way to performance test a patch like this?
Sorry I didn't mention performance before. I get the same results on sunspider and v8-v6 with this patch as before, within the millisecond. The binary is some 10 kB smaller or so, with a -g -O2 build with gcc-4.6. I can see if I can get bencher working tomorrow, if that would be useful. (I had tried before, with no success.)
Do run-sunspider --parse-only And also in the past i've thrown large amounts of JS into a single file and directly measured parse time.
It is really difficult to say anything with these results. The error bars are too high. I did apply something like bug 71801 to my sunspider before running the tests in order to get higher precision timers. In any case, performance seems to be a wash. TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: ?? 14.1ms +/- 8.9% 14.1ms +/- 7.3% not conclusive: might be *1.003x as slow* ============================================================================= jquery: ?? 2.2ms +/- 14.1% 2.4ms +/- 23.5% not conclusive: might be *1.055x as slow* 1.3.2: ?? 2.2ms +/- 14.1% 2.4ms +/- 23.5% not conclusive: might be *1.055x as slow* mootools: - 2.5ms +/- 23.4% 2.5ms +/- 16.4% 1.2.2-core-nc: - 2.5ms +/- 23.4% 2.5ms +/- 16.4% prototype: - 2.7ms +/- 14.0% 2.6ms +/- 6.9% 1.6.0.3: - 2.7ms +/- 14.0% 2.6ms +/- 6.9% concat: ?? 6.6ms +/- 4.6% 6.7ms +/- 6.5% not conclusive: might be *1.018x as slow* jquery-mootools-prototype: ?? 6.6ms +/- 4.6% 6.7ms +/- 6.5% not conclusive: might be *1.018x as slow*
Attaching a rebase on top of bug 79776.
Created attachment 129261 [details] Patch
Comment on attachment 129261 [details] Patch Flipping r? again to run the queues
Comment on attachment 129261 [details] Patch I believe this patch still applies. It is performance-neutral. On my parse test, trunk's best time over 10 tries is 5.983s, and with this patch is 6.002s.
Created attachment 130642 [details] new patch to run the ews
Comment on attachment 130642 [details] new patch to run the ews Attachment 130642 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11852187 New failing tests: css3/filters/effect-contrast-hw.html
Comment on attachment 130642 [details] new patch to run the ews View in context: https://bugs.webkit.org/attachment.cgi?id=130642&action=review r=me > Source/JavaScriptCore/parser/Nodes.h:1416 > + void setScopeFlags(ScopeFlags scopeFlags) { m_scopeFlags |= scopeFlags; } A setter with this name would traditionally overwrite the pre-existing value. I'd suggest "addScopeFlags" instead.
Created attachment 131015 [details] Patch for landing
Comment on attachment 131015 [details] Patch for landing Rejecting attachment 131015 [details] from commit-queue. New failing tests: perf/object-keys.html Full output: http://queues.webkit.org/results/11911506
Comment on attachment 131015 [details] Patch for landing Clearing flags on attachment: 131015 Committed r110287: <http://trac.webkit.org/changeset/110287>
All reviewed patches have been landed. Closing bug.
I landed this patch because I couldn't see what the chromium bot would have to say about JSC performance. If I did so in error, please let me know. Thanks.
This patch broke templating on qq.com -- it would be really good if in future refactoring patches were the smallest they could be, and were landed one piece at a time. Currently i'm looking at a huge pile of code to determine where the functionality was changed.
Rolled out in r116372
Hi Oliver, I'm sorry I didn't catch this when you were having problems. Are you certain that this patch caused a problem? Do you have any more details? I can visit qq.com but I have no idea how to tell that it's correct or not.
It's been quite a while. Can this bug be closed again?