Summary: | Refactor code feature analysis in the parser | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andy Wingo <wingo> | ||||||||||
Component: | JavaScriptCore | Assignee: | Andy Wingo <wingo> | ||||||||||
Status: | REOPENED --- | ||||||||||||
Severity: | Normal | CC: | barraclough, dglazkov, emanuele.aina, ggaren, oliver, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 79776 | ||||||||||||
Bug Blocks: | 74509 | ||||||||||||
Attachments: |
|
Description
Andy Wingo
2012-02-21 08:23:48 PST
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* 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? |