Bug 79112

Summary: Refactor code feature analysis in the parser
Product: WebKit Reporter: Andy Wingo <wingo>
Component: JavaScriptCoreAssignee: 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 Flags
Patch
none
Patch
none
new patch to run the ews
none
Patch for landing none

Andy Wingo
Reported 2012-02-21 08:23:48 PST
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.
Attachments
Patch (55.21 KB, patch)
2012-02-21 08:27 PST, Andy Wingo 		no flags
DetailsFormatted DiffDiff
Patch (54.87 KB, patch)
2012-02-28 08:36 PST, Andy Wingo 		no flags
DetailsFormatted DiffDiff
new patch to run the ews (54.86 KB, patch)
2012-03-07 10:20 PST, Andy Wingo 		no flags
DetailsFormatted DiffDiff
Patch for landing (54.88 KB, patch)
2012-03-09 02:04 PST, Andy Wingo 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Andy Wingo
Comment 1 2012-02-21 08:27:16 PST
Created attachment 127976 [details] Patch
Geoffrey Garen
Comment 2 2012-02-21 10:51:23 PST
Oliver, what's the best way to performance test a patch like this?
Andy Wingo
Comment 3 2012-02-21 14:17:40 PST
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.)
Oliver Hunt
Comment 4 2012-02-21 14:34:02 PST
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.
Andy Wingo
Comment 5 2012-02-27 03:13:14 PST
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*
Andy Wingo
Comment 6 2012-02-28 08:33:36 PST
Attaching a rebase on top of bug 79776.
Andy Wingo
Comment 7 2012-02-28 08:36:47 PST
Created attachment 129261 [details] Patch
Andy Wingo
Comment 8 2012-03-07 10:16:02 PST
Comment on attachment 129261 [details] Patch Flipping r? again to run the queues
Andy Wingo
Comment 9 2012-03-07 10:17:56 PST
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.
Andy Wingo
Comment 10 2012-03-07 10:20:17 PST
Created attachment 130642 [details] new patch to run the ews
WebKit Review Bot
Comment 11 2012-03-07 11:24:08 PST
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
Geoffrey Garen
Comment 12 2012-03-07 18:29:11 PST
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.
Andy Wingo
Comment 13 2012-03-09 02:04:07 PST
Created attachment 131015 [details] Patch for landing
WebKit Review Bot
Comment 14 2012-03-09 03:32:35 PST
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
Andy Wingo
Comment 15 2012-03-09 04:07:03 PST
Comment on attachment 131015 [details] Patch for landing Clearing flags on attachment: 131015 Committed r110287: <http://trac.webkit.org/changeset/110287>
Andy Wingo
Comment 16 2012-03-09 04:07:14 PST
All reviewed patches have been landed. Closing bug.
Andy Wingo
Comment 17 2012-03-09 04:09:37 PST
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.
Oliver Hunt
Comment 18 2012-05-07 15:32:23 PDT
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.
Oliver Hunt
Comment 19 2012-05-07 16:37:15 PDT
Rolled out in r116372
Andy Wingo
Comment 20 2012-05-21 07:01:30 PDT
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.
Emanuele Aina
Comment 21 2015-11-30 07:01:39 PST
It's been quite a while. Can this bug be closed again?
Note You need to log in before you can comment on or make changes to this bug.