Bug 155862 – Make RegExp.prototype.test spec compliant

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

Created attachment 274871 [details]
WIP

WIP is here mostly for the C++ RegExpExec function.
But this patch is wrong w.r.t how we make test into
an intrinsic.

[Radar WebKit Bug Importer]

<rdar://problem/25770509>

[Mark Lam]

Created attachment 277402 [details]
work in progress: waiting for bug 156562 for accepted pattern for making test an intrinsic.

[Yusuke Suzuki]

NOTE: This is the last test case failing in compat-table :) https://github.com/kangax/compat-table/commit/faddf0016be9959761ea5ad93d25e6e4d41c3196

[Mark Lam]

Created attachment 277597 [details]
work in progress.

This patch passes all tests.  It has the RegExpTest intrinsics implemented but perf is still bad.  The current regression is as follows:
                                            base                      new                                        
simple-regexp-test-folding-fail        3.9705+-0.0665     !     19.8793+-0.4888        ! definitely 5.0068x slower
simple-regexp-test-folding             4.8262+-0.1007     !     21.2555+-0.3805        ! definitely 4.4042x slower

Before the intrinsics were implemented, we were getting about a 10x perf regression on these microbenchmarks.  Now, we're around 5x slower.  I still need to dig in a bit more to investigate what's causing the regression.

[WebKit Commit Bot]

Attachment 277597 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2239:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Here's the relevant IR before lowering to B3:

Old code:
  ...
  28:<!0:->	LoopHint(MustGen, W:SideState, bc#13)
  89:<!0:->	KillStack(MustGen, loc10, W:SideState, ClobbersExit, bc#14)
  30:<!0:->	ZombieHint(MustGen, loc10, W:SideState, ClobbersExit, bc#14, ExitInvalid)
  90:<!0:->	KillStack(MustGen, loc7, W:SideState, ClobbersExit, bc#17)
  35:<!0:->	ZombieHint(MustGen, loc7, W:SideState, ClobbersExit, bc#17, ExitInvalid)
  91:<!0:->	KillStack(MustGen, loc9, W:SideState, ClobbersExit, bc#26)
  38:<!0:->	ZombieHint(MustGen, loc9, W:SideState, ClobbersExit, bc#26, ExitInvalid)
  97:<!0:->	StoreBarrier(KnownCell:@60, MustGen, W:SideState, bc#29)
  64:<!0:->	RecordRegExpCachedResult(KnownCell:@60, KnownCell:@61, KnownCell:@37, KnownInt32:@22, KnownInt32:@63, MustGen|VarArgs, W:RegExpState, ClobbersExit, bc#29)
  92:<!0:->	KillStack(MustGen, loc7, W:SideState, ClobbersExit, bc#29, ExitInvalid)
  43:<!0:->	ZombieHint(MustGen, loc7, W:SideState, ClobbersExit, bc#29, ExitInvalid)
  50:<!3:->	ArithAdd(Int32:Kill:@73, Int32:@49, Number|MustGen|UseAsOther, Int32, Unchecked, Exits, bc#38)
  93:<!0:->	KillStack(MustGen, loc5, W:SideState, ClobbersExit, bc#38)
  51:<!0:->	MovHint(Untyped:@50, MustGen, loc5, W:SideState, ClobbersExit, bc#38, ExitInvalid)
  54:< 1:->	CompareLess(Int32:@50, Int32:@25, Boolean|UseAsOther, Bool, Exits, bc#40)
  55:<!0:->	Branch(KnownBoolean:Kill:@54, MustGen, T:#1/w:10.000000, F:#4/w:1.000000, W:SideState, bc#40, ExitInvalid)
  ...

New code:
  ...
  28:<!0:->	LoopHint(MustGen, W:SideState, bc#13)
  29:< 2:->	NewRegexp(JS|UseAsOther, Regexpobject, R:HeapObjectCount, W:HeapObjectCount, Exits, bc#14)           <============= NEW
  92:<!0:->	KillStack(MustGen, loc10, W:SideState, ClobbersExit, bc#14)
  30:<!0:->	MovHint(Untyped:@29, MustGen, loc10, W:SideState, ClobbersExit, bc#14, ExitInvalid)
  93:<!0:->	KillStack(MustGen, loc7, W:SideState, ClobbersExit, bc#17)
  35:<!0:->	MovHint(Untyped:@34, MustGen, loc7, W:SideState, ClobbersExit, bc#17, ExitInvalid)
  94:<!0:->	KillStack(MustGen, loc9, W:SideState, ClobbersExit, bc#26)
  38:<!0:->	MovHint(Untyped:@37, MustGen, loc9, W:SideState, ClobbersExit, bc#26, ExitInvalid)
  41:< 1:->	TryGetById(Cell:Kill:@29, JS|UseAsOther, TopEmpty, id1{exec}, R:Heap, Exits, bc#29)                  <============= NEW
  42:<!0:->	CheckCell(Check:Cell:Kill:@41, MustGen, <0x10d5d70a0, Function>, <host function>, Exits, bc#29)      <============= NEW
 100:<!0:->	StoreBarrier(KnownCell:@63, MustGen, W:SideState, bc#29)
  67:<!0:->	RecordRegExpCachedResult(KnownCell:@63, KnownCell:@64, KnownCell:@37, KnownInt32:@22, KnownInt32:@66, MustGen|VarArgs, W:RegExpState, ClobbersExit, bc#29)
  95:<!0:->	KillStack(MustGen, loc7, W:SideState, ClobbersExit, bc#29, ExitInvalid)
  46:<!0:->	ZombieHint(MustGen, loc7, W:SideState, ClobbersExit, bc#29, ExitInvalid)
  53:<!3:->	ArithAdd(Int32:Kill:@76, Int32:@52, Number|MustGen|UseAsOther, Int32, Unchecked, Exits, bc#38)
  96:<!0:->	KillStack(MustGen, loc5, W:SideState, ClobbersExit, bc#38)
  54:<!0:->	MovHint(Untyped:@53, MustGen, loc5, W:SideState, ClobbersExit, bc#38, ExitInvalid)
  57:< 1:->	CompareLess(Int32:@53, Int32:@25, Boolean|UseAsOther, Bool, Exits, bc#40)
  58:<!0:->	Branch(KnownBoolean:Kill:@57, MustGen, T:#1/w:10.000000, F:#4/w:1.000000, W:SideState, bc#40, ExitInvalid)
  ...

Because of the need for the TryGetById, we'll end up keeping the NewRegexp alive.  We don't currently constant fold away the TryGetById.  This ends up doing work in the generated code that accounts for the 5x regression in the simple-regexp-test-folding* micro-benchmarks.  We are successful in folding away the call to RegExp.prototype.test.  Fil said that this is sufficient.  The work to constant fold TryGetByid is out of scope for this bug.

Currently running full benchmarks on the patch.  If all goes well, will submit for review.

[Filip Pizlo]

I recommend adding a new, modified version of this microbenchmark that hoists the regexp allocation out of the loop.

This way, we'll have both a test to remind us to constant-fold tryGetById (the current version of the test) and a new test that isolates the profitability of constant-folding RegExpTest (the new version of the test).  Once all bugs are fixed, the two tests should perform the same, but it's not a top priority.

[Mark Lam]

Created attachment 277651 [details]
proposed patch.

Rebased patch + real ChangeLog.  Still waiting for benchmark results.

[WebKit Commit Bot]

Attachment 277651 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2239:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

(In reply to comment #8)
> I recommend adding a new, modified version of this microbenchmark that
> hoists the regexp allocation out of the loop.

Great idea.  Here are the new micro-benchmark results:

                                                     base                      new                                        
simple-regexp-test-folding-fail-with-hoisted-regexp
                                               18.3394+-0.1967     !     20.4064+-0.2284        ! definitely 1.1127x slower
simple-regexp-test-folding-fail                 4.1179+-0.1119     !     20.1755+-0.2900        ! definitely 4.8995x slower
simple-regexp-test-folding-with-hoisted-regexp
                                               17.9460+-0.3154     !     20.1089+-0.2910        ! definitely 1.1205x slower
simple-regexp-test-folding                      4.9317+-0.0701     !     21.8603+-0.2295        ! definitely 4.4326x slower
  
With the RegExp construction in the loop, we have the 5x regression.
With the RegExp construction hoisted out of the loop, we have about a 12% regression.

The difference is because in the new code, we still need to do work to do the TryGetById and CheckCell.  Here's the IR before lowering to B3:

  33:<!0:->	LoopHint(MustGen, W:SideState, bc#16)
  ...
  46:< 1:->	TryGetById(Cell:@70, JS|UseAsOther, TopEmpty, id1{exec}, R:Heap, Exits, bc#32)                     <======== NEW
  47:<!0:->	CheckCell(Check:Cell:Kill:@46, MustGen, <0x10ddd70a0, Function>, <host function>, Exits, bc#32)    <======== NEW

  50:<!0:->	RegExpTest(KnownCell:@49, RegExpObject:@70, String:@42, JS|MustGen|PureInt, Bool, R:RegExpObject_lastIndex,RegExpState, W:RegExpObject_lastIndex,RegExpState, Exits, ClobbersExit, bc#32)  predicting Bool
  98:<!0:->	KillStack(MustGen, loc8, W:SideState, ClobbersExit, bc#32, ExitInvalid)
  51:<!0:->	ZombieHint(MustGen, loc8, W:SideState, ClobbersExit, bc#32, ExitInvalid)
  58:<!3:->	ArithAdd(Int32:Kill:@89, Int32:@57, Number|MustGen|UseAsOther, Int32, CheckOverflow, Exits, bc#41)
  99:<!0:->	KillStack(MustGen, loc5, W:SideState, ClobbersExit, bc#41)
  59:<!0:->	MovHint(Untyped:@58, MustGen, loc5, W:SideState, ClobbersExit, bc#41, ExitInvalid)
  62:< 1:->	CompareLess(Int32:@58, Int32:@61, Boolean|UseAsOther, Bool, Exits, bc#43)
  63:<!0:->	Branch(KnownBoolean:Kill:@62, MustGen, T:#1/w:10.000000, F:#3/w:1.000000, W:SideState, bc#43, ExitInvalid)

In this test, we're not folding away the RegExpTest though in both the old and new code.

[Mark Lam]

(In reply to comment #11)
>   50:<!0:->	RegExpTest(KnownCell:@49, RegExpObject:@70, String:@42,
> JS|MustGen|PureInt, Bool, R:RegExpObject_lastIndex,RegExpState,
> W:RegExpObject_lastIndex,RegExpState, Exits, ClobbersExit, bc#32) 
> predicting Bool
...
> 
> In this test, we're not folding away the RegExpTest though in both the old
> and new code.

Saam looked at this with me and noticed that the reason RegExpTest is not folded is because we're OSR entering into the loop, and the loop doesn't know that the RegExp object is constant.

Here's the original test:

    (function() {
        var regExp = /foo/;
        for (var i = 0; i < 1000000; ++i)
            regExp.test("foo");
    })();

Here's the new version of the test that will allow us to fold RegExpTest:

    (function() {
        for (var j = 0; j < 10; ++j) {
            (function () {
                var regExp = /foo/;
                for (var i = 0; i < 100000; ++i)
                    regExp.test("foo");
            })();
        }
    })();

With the new versions of the tests with the RegExp object hoisted out of the loop, the micro-benchmark results are:

                                                     base                      new                                        
simple-regexp-test-folding-fail-with-hoisted-regexp
                                                9.7717+-0.0654     !     11.6440+-0.3730        ! definitely 1.1916x slower
simple-regexp-test-folding-fail                 3.9023+-0.0503     !     18.8741+-0.1755        ! definitely 4.8367x slower
simple-regexp-test-folding-with-hoisted-regexp
                                               10.3621+-0.0655     !     12.4605+-0.1900        ! definitely 1.2025x slower
simple-regexp-test-folding                      4.6939+-0.0462     !     20.3603+-0.1146        ! definitely 4.3376x slower


With the new code, the IR before lowering to B3 is:

  33:<!0:->	LoopHint(MustGen, W:SideState, bc#16)
  98:<!0:->	KillStack(MustGen, loc12, W:SideState, ClobbersExit, bc#17)
  35:<!0:->	MovHint(Untyped:@24, MustGen, loc12, W:SideState, ClobbersExit, bc#17, ExitInvalid)
  99:<!0:->	KillStack(MustGen, loc8, W:SideState, ClobbersExit, bc#20)
  40:<!0:->	MovHint(Untyped:@39, MustGen, loc8, W:SideState, ClobbersExit, bc#20, ExitInvalid)
 100:<!0:->	KillStack(MustGen, loc11, W:SideState, ClobbersExit, bc#29)
  43:<!0:->	MovHint(Untyped:@42, MustGen, loc11, W:SideState, ClobbersExit, bc#29, ExitInvalid)
  46:< 1:->	TryGetById(Cell:@24, JS|UseAsOther, TopEmpty, id1{exec}, R:Heap, Exits, bc#32)                        <======= NEW
  47:<!0:->	CheckCell(Check:Cell:Kill:@46, MustGen, <0x1139d70a0, Function>, <host function>, Exits, bc#32)       <======= NEW
 118:<!0:->	StoreBarrier(KnownCell:@107, MustGen, W:SideState, bc#32)
 111:<!0:->	RecordRegExpCachedResult(KnownCell:@107, KnownCell:@108, KnownCell:@42, KnownInt32:@27, KnownInt32:@110, MustGen|VarArgs, W:RegExpState, ClobbersExit, bc#32)        <========= RegExpTest is folded.
 101:<!0:->	KillStack(MustGen, loc8, W:SideState, ClobbersExit, bc#32, ExitInvalid)
  51:<!0:->	ZombieHint(MustGen, loc8, W:SideState, ClobbersExit, bc#32, ExitInvalid)
  58:<!3:->	ArithAdd(Int32:Kill:@80, Int32:@57, Number|MustGen|UseAsOther, Int32, Unchecked, Exits, bc#41)
 102:<!0:->	KillStack(MustGen, loc5, W:SideState, ClobbersExit, bc#41)
  59:<!0:->	MovHint(Untyped:@58, MustGen, loc5, W:SideState, ClobbersExit, bc#41, ExitInvalid)
  62:< 1:->	CompareLess(Int32:@58, Int32:@30, Boolean|UseAsOther, Bool, Exits, bc#43)
  63:<!0:->	Branch(KnownBoolean:Kill:@62, MustGen, T:#1/w:10.000000, F:#4/w:1.000000, W:SideState, bc#43, ExitInvalid)

[Mark Lam]

Created attachment 277660 [details]
proposed patch with the new hoisted regexp tests.

[Mark Lam]

Created attachment 277661 [details]
x86_64 benchmark results.

Benchmark results shows that perf is neutral except for the simple-regexp-test-folding* micro-benchmarks which have a 5% regression as previously stated.

[WebKit Commit Bot]

Attachment 277660 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2239:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 277711 [details]
in-browser sunspider 1.0.2 results on old code

[Mark Lam]

Created attachment 277712 [details]
in-browser sunspider 1.0.2 results on new code

[Mark Lam]

In browser sunspider results are neutral.

Jetstream results also appear to be neutral.  The score fluctuates up and down by a few points (on both the old and new code) but they are both appear to be near each other's scores.  For example:

Old code:
    Score 184.50 ± 2.7204

    Benchmark         Average Score     Benchmark     Average Score   Benchmark       Average Score
    Latency           119.99 ± 2.3427   regex-dna     118.3 ± 15.61   gcc-loops.cpp   383.3 ± 21.08
    3d-cube           113.5 ± 7.133     splay-latency 165.7 ± 3.484   hash-map        333.7 ± 10.24
    3d-raytrace       113.4 ± 5.199     tagcloud      145.2 ± 37.59   mandreel        289.1 ± 14.04
    base64            80.56 ± 14.79     typescript    128.7 ± 9.043   n-body.c        243.2 ± 11.67
    cdjs              183.9 ± 44.67     Throughput    257.28 ± 6.9501 navier-stokes   176.2 ± 5.632
    code-first-load   120.3 ± 1.440     bigfib.cpp    318.7 ± 45.19   pdfjs           198.2 ± 10.79
    code-multi-load   111.1 ± 5.464     box2d         225.9 ± 18.56   proto-raytracer 308.3 ± 5.182
    crypto-aes        129.8 ± 3.637     container.cpp 294.1 ± 13.73   quicksort.c     266.6 ± 2.175
    crypto-md5        99.45 ± 2.350     crypto        181.0 ± 9.191   regexp-2010     297.2 ± 2.023
    crypto-sha1       100.9 ± 6.996     delta-blue    280.0 ± 5.110   richards        206.1 ± 7.673
    date-format-tofte 90.47 ± 3.063     dry.c         318.6 ± 43.54   splay           250.8 ± 11.13
    date-format-xparb 124.8 ± 24.96     earley-boyer  177.1 ± 7.652   towers.c        255.5 ± 18.35
    mandreel-latency  164.4 ± 15.86     float-mm.c    333.8 ± 14.60   zlib            217.9 ± 5.861
    n-body            99.45 ± 2.350     gbemu         240.6 ± 14.15   Geometric Mean  184.50 ± 2.7204

New code:
    Score 188.95 ± 0.65793

    Benchmark          Average Score    Benchmark     Average Score   Benchmark       Average Score
    Latency            123.45 ± 3.1012  regex-dna     125.6 ± 2.495   gcc-loops.cpp   390.1 ± 9.796
    3d-cube            110.0 ± 13.99    splay-latency 193.6 ± 10.36   hash-map        329.8 ± 14.68
    3d-raytrace        112.3 ± 1.121    tagcloud      152.4 ± 6.790   mandreel        300.6 ± 18.21
    base64             80.61 ± 8.616    typescript    134.9 ± 10.35   n-body.c        242.0 ± 12.24
    cdjs               188.1 ± 12.06    Throughput    262.56 ± 4.0256 navier-stokes   175.5 ± 4.897
    code-first-load    123.6 ± 5.641    bigfib.cpp    314.4 ± 10.81   pdfjs           199.9 ± 4.839
    code-multi-load    115.2 ± 1.724    box2d         231.2 ± 7.873   proto-raytracer 306.3 ± 3.891
    crypto-aes         127.7 ± 1.766    container.cpp 290.4 ± 20.66   quicksort.c     266.8 ± 1.428
    crypto-md5         100.0 ± 0.000    crypto        182.9 ± 12.09   regexp-2010     294.7 ± 15.74
    crypto-sha1        101.7 ± 3.583    delta-blue    277.7 ± 4.190   richards        202.6 ± 7.360
    date-format-tofte  91.14 ± 14.74    dry.c         309.1 ± 66.86   splay           265.5 ± 22.37
    date-format-xparb  130.0 ± 3.969    earley-boyer  169.5 ± 10.91   towers.c        259.3 ± 4.292
    mandreel-latency   176.8 ± 13.68    float-mm.c    339.0 ± 9.809   zlib            216.7 ± 14.39
    n-body             99.45 ± 2.350    gbemu         369.0 ± 12.60   Geometric Mean  188.95 ± 0.65793

[Saam Barati]

Comment on attachment 277660 [details]
proposed patch with the new hoisted regexp tests.

View in context: https://bugs.webkit.org/attachment.cgi?id=277660&action=review

leaving as r? because I have a few questions. Mostly LGTM

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2224
> +        Node* regExpObject = get(virtualRegisterForArgument(0, registerOffset));

You should only add this get to the graph if we know we're going to make it an intrinsic (i.e, not return false).

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2228
> +            if (m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCell))
> +                return false;

Couldn't we also exit for BadType here? Do we want to take that into account?

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:658
> +        GlobalPropertyInfo(vm.propertyNames->regExpTestFastPrivateName, JSFunction::create(vm, this, 1, String(), regExpProtoFuncTestFast, RegExpTestIntrinsic), DontEnum | DontDelete | ReadOnly),

shouldn't this be RegExpTestFastIntrinsic?

[Mark Lam]

(In reply to comment #19)
> Comment on attachment 277660 [details]
> proposed patch with the new hoisted regexp tests.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277660&action=review
> 
> leaving as r? because I have a few questions. Mostly LGTM
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2224
> > +        Node* regExpObject = get(virtualRegisterForArgument(0, registerOffset));
> 
> You should only add this get to the graph if we know we're going to make it
> an intrinsic (i.e, not return false).

Good point.  Will fix.

> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2228
> > +            if (m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCell))
> > +                return false;
> 
> Couldn't we also exit for BadType here? Do we want to take that into account?

Let me look into that.

> > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:658
> > +        GlobalPropertyInfo(vm.propertyNames->regExpTestFastPrivateName, JSFunction::create(vm, this, 1, String(), regExpProtoFuncTestFast, RegExpTestIntrinsic), DontEnum | DontDelete | ReadOnly),
> 
> shouldn't this be RegExpTestFastIntrinsic?

Eeek.  Thanks for catching that.  Will fix and redo tests and benchmarks.

[Mark Lam]

(In reply to comment #20)
> (In reply to comment #19)
> > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2228
> > > +            if (m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCell))
> > > +                return false;
> > 
> > Couldn't we also exit for BadType here? Do we want to take that into account?
> 
> Let me look into that.

Here's what I found:
1. handleInlining() will check "if (!callLinkStatus.couldTakeSlowPath() && callLinkStatus.size() == 1)" before attempting to inline the intrinsic.
2. If we ever bail due to BadType at this call site, callLinkStatus.couldTakeSlowPath() will return true.
   This ensures that we don't try to inline it again.

In addition, if we ever change the inlining code to support multiple call targets in the future, we don't want to invalidate the inlining of this intrinsic if we happened to encounter a BadType.

So, in summary, we don't need to and shouldn't add a BadType check here.  The existing callLinkStatus.couldTakeSlowPath() check is sufficient.

[Filip Pizlo]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2228
> > > > +            if (m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCell))
> > > > +                return false;
> > > 
> > > Couldn't we also exit for BadType here? Do we want to take that into account?
> > 
> > Let me look into that.
> 
> Here's what I found:
> 1. handleInlining() will check "if (!callLinkStatus.couldTakeSlowPath() &&
> callLinkStatus.size() == 1)" before attempting to inline the intrinsic.
> 2. If we ever bail due to BadType at this call site,
> callLinkStatus.couldTakeSlowPath() will return true.
>    This ensures that we don't try to inline it again.
> 
> In addition, if we ever change the inlining code to support multiple call
> targets in the future, we don't want to invalidate the inlining of this
> intrinsic if we happened to encounter a BadType.
> 
> So, in summary, we don't need to and shouldn't add a BadType check here. 
> The existing callLinkStatus.couldTakeSlowPath() check is sufficient.

I agree with this analysis.

[Mark Lam]

Created attachment 277740 [details]
proposed patch w/ Saam's feedback applied.

[Mark Lam]

Created attachment 277741 [details]
in-browser sunspider 1.0.2 results on old code

[Mark Lam]

Created attachment 277742 [details]
in-browser sunspider 1.0.2 results on new code

[WebKit Commit Bot]

Attachment 277740 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2238:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2228
> > > > +            if (m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCell))
> > > > +                return false;
> > > 
> > > Couldn't we also exit for BadType here? Do we want to take that into account?
> > 
> > Let me look into that.
> 
> Here's what I found:
> 1. handleInlining() will check "if (!callLinkStatus.couldTakeSlowPath() &&
> callLinkStatus.size() == 1)" before attempting to inline the intrinsic.
> 2. If we ever bail due to BadType at this call site,
> callLinkStatus.couldTakeSlowPath() will return true.
>    This ensures that we don't try to inline it again.
> 
> In addition, if we ever change the inlining code to support multiple call
> targets in the future, we don't want to invalidate the inlining of this
> intrinsic if we happened to encounter a BadType.
> 
> So, in summary, we don't need to and shouldn't add a BadType check here. 
> The existing callLinkStatus.couldTakeSlowPath() check is sufficient.
Interesting. It looks like 'StringPrototypeReplaceIntrinsic' is wrong for checking
for BadType exits then.

[Mark Lam]

Created attachment 277753 [details]
x86_64 benchmark results.

Perf appears to be neutral, again with the exception of the 2 simple-regexp-test-folding* microbenchmarks.

LongSpider's 3d-cube regression is sometimes reproducible if I re-run the test harness (with --outer 8) with just that test.  However, the test itself does not use RegExp.prototype.test.  So, I'll consider this noise.

There is one other exception though: JSRegress's math-with-out-of-bounds-array-values is consistently about 10-11% faster no matter how many times I re-run it.  That test also does not use RegExp.prototype.test.  I have no idea how this could have happened.

Anyway, perf is neutral on all important benchmarks.

[Mark Lam]

JetStream numbers on the latest (i.e. 2016-49-29 15:48 PDT) patch are also neutral:

Old code:
    Score 190.79 ± 1.3665

    Benchmark           Average Score   Benchmark       Average Score   Benchmark       Average Score
    Latency             124.03 ± 1.1093 regex-dna       124.9 ± 3.785   gcc-loops.cpp   389.8 ± 3.140
    3d-cube             115.5 ± 22.54   splay-latency   194.1 ± 13.04   hash-map        339.1 ± 14.44
    3d-raytrace         113.1 ± 2.289   tagcloud        150.7 ± 23.73   mandreel        303.0 ± 13.49
    base64              81.29 ± 1.124   typescript      135.7 ± 1.001   n-body.c        244.4 ± 3.383
    cdjs                185.3 ± 1.527   Throughput      266.13 ± 3.1652 navier-stokes   177.9 ± 3.383
    code-first-load     124.3 ± 0.4246  bigfib.cpp      302.5 ± 7.990   pdfjs           206.8 ± 1.001
    code-multi-load     115.0 ± 2.972   box2d           229.8 ± 0.8704  proto-raytracer 309.7 ± 8.418
    crypto-aes          129.0 ± 3.637   container.cpp   296.4 ± 3.204   quicksort.c     267.8 ± 3.615
    crypto-md5          100.0 ± 0.000   crypto          186.6 ± 3.534   regexp-2010     299.8 ± 5.900
    crypto-sha1         100.9 ± 6.996   delta-blue      280.3 ± 5.327   richards        207.0 ± 18.57
    date-format-tofte   94.96 ± 6.326   dry.c           326.0 ± 47.76   splay           265.7 ± 14.34
    date-format-xparb   131.0 ± 29.91   earley-boyer    173.6 ± 3.718   towers.c        260.5 ± 8.724
    mandreel-latency    174.0 ± 26.39   float-mm.c      342.3 ± 0.6296  zlib            221.9 ± 4.360
    n-body              100.0 ± 0.000   gbemu           379.2 ± 15.20   Geometric Mean  190.79 ± 1.3665

New code:
    Score 190.99 ± 1.0815

    Benchmark           Average Score   Benchmark       Average Score   Benchmark       Average Score
    Latency             124.09 ± 1.1507 regex-dna       124.3 ± 7.522   gcc-loops.cpp   392.4 ± 3.327
    3d-cube             109.3 ± 9.194   splay-latency   195.3 ± 10.98   hash-map        325.2 ± 36.04
    3d-raytrace         109.8 ± 8.434   tagcloud        158.2 ± 4.268   mandreel        304.7 ± 5.919
    base64              84.00 ± 0.000   typescript      132.9 ± 6.359   n-body.c        245.1 ± 2.654
    cdjs                186.8 ± 27.96   Throughput      266.51 ± 2.8073 navier-stokes   177.8 ± 3.212
    code-first-load     124.1 ± 0.5710  bigfib.cpp      321.5 ± 1.714   pdfjs           204.3 ± 3.084
    code-multi-load     113.5 ± 2.092   box2d           227.9 ± 3.253   proto-raytracer 313.9 ± 2.247
    crypto-aes          127.7 ± 1.766   container.cpp   298.2 ± 7.238   quicksort.c     267.8 ± 5.045
    crypto-md5          100.0 ± 0.000   crypto          188.9 ± 0.8679  regexp-2010     297.2 ± 2.875
    crypto-sha1         102.5 ± 0.000   delta-blue      283.2 ± 10.42   richards        213.2 ± 6.081
    date-format-tofte   91.29 ± 2.079   dry.c           310.2 ± 62.43   splay           265.9 ± 20.74
    date-format-xparb   135.9 ± 11.52   earley-boyer    175.8 ± 1.957   towers.c        260.5 ± 1.252
    mandreel-latency    178.9 ± 2.267   float-mm.c      341.6 ± 1.254   zlib            223.0 ± 1.228
    n-body              100.0 ± 0.000   gbemu           375.4 ± 6.460   Geometric Mean  190.99 ± 1.0815

[Saam Barati]

Comment on attachment 277740 [details]
proposed patch w/ Saam's feedback applied.

View in context: https://bugs.webkit.org/attachment.cgi?id=277740&action=review

r=me

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2238
> +            auto isRegExpPropertySame = [&] (JSValue primordialProperty, UniquedStringImpl* propertyUID) {

Nit: It might be worth converging this with other implementation. Up to you

[Mark Lam]

(In reply to comment #30)
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2238
> > +            auto isRegExpPropertySame = [&] (JSValue primordialProperty, UniquedStringImpl* propertyUID) {
> 
> Nit: It might be worth converging this with other implementation. Up to you

I'll look into doing that in a subsequent patch.

[Mark Lam]

Thanks for the review.  Landed in r200272: <http://trac.webkit.org/r200272>.