Bug 79112 – Refactor code feature analysis in the parser

• Status: REOPENED
• Component: JavaScriptCore

[Andy Wingo]

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.

[Andy Wingo]

Created attachment 127976 [details]
Patch

[Geoffrey Garen]

Oliver, what's the best way to performance test a patch like this?

[Andy Wingo]

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]

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]

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]

Attaching a rebase on top of bug 79776.

[Andy Wingo]

Created attachment 129261 [details]
Patch

[Andy Wingo]

Comment on attachment 129261 [details]
Patch

Flipping r? again to run the queues

[Andy Wingo]

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]

Created attachment 130642 [details]
new patch to run the ews

[WebKit Review Bot]

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 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]

Created attachment 131015 [details]
Patch for landing

[WebKit Review Bot]

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 on attachment 131015 [details]
Patch for landing

Clearing flags on attachment: 131015

Committed r110287: <http://trac.webkit.org/changeset/110287>

[Andy Wingo]

All reviewed patches have been landed.  Closing bug.

[Andy Wingo]

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]

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]

Rolled out in r116372

[Andy Wingo]

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]

It's been quite a while. Can this bug be closed again?