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    
Description Flags
new patch to run the ews
Patch for landing none

Description Andy Wingo 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.
Comment 1 Andy Wingo 2012-02-21 08:27:16 PST
Created attachment 127976 [details]
Comment 2 Geoffrey Garen 2012-02-21 10:51:23 PST
Oliver, what's the best way to performance test a patch like this?
Comment 3 Andy Wingo 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.)
Comment 4 Oliver Hunt 2012-02-21 14:34:02 PST
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.
Comment 5 Andy Wingo 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%                   -                  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*
Comment 6 Andy Wingo 2012-02-28 08:33:36 PST
Attaching a rebase on top of bug 79776.
Comment 7 Andy Wingo 2012-02-28 08:36:47 PST
Created attachment 129261 [details]
Comment 8 Andy Wingo 2012-03-07 10:16:02 PST
Comment on attachment 129261 [details]

Flipping r? again to run the queues
Comment 9 Andy Wingo 2012-03-07 10:17:56 PST
Comment on attachment 129261 [details]

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.
Comment 10 Andy Wingo 2012-03-07 10:20:17 PST
Created attachment 130642 [details]
new patch to run the ews
Comment 11 WebKit Review Bot 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:
Comment 12 Geoffrey Garen 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


> 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.
Comment 13 Andy Wingo 2012-03-09 02:04:07 PST
Created attachment 131015 [details]
Patch for landing
Comment 14 WebKit Review Bot 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:
Full output: http://queues.webkit.org/results/11911506
Comment 15 Andy Wingo 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>
Comment 16 Andy Wingo 2012-03-09 04:07:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Andy Wingo 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.
Comment 18 Oliver Hunt 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.
Comment 19 Oliver Hunt 2012-05-07 16:37:15 PDT
Rolled out in r116372
Comment 20 Andy Wingo 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.
Comment 21 Emanuele Aina 2015-11-30 07:01:39 PST
It's been quite a while. Can this bug be closed again?