ValueAdd should be constant folded if the operands are constant String,Primitive or Primitive,String

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

This comes up in Dromaeo

[Yusuke Suzuki]

NICE!

[Saam Barati]

Created attachment 291110 [details]
WIP

[Saam Barati]

Created attachment 291112 [details]
WIP

[Saam Barati]

Created attachment 291215 [details]
patch

[Saam Barati]

Two Dromaeo runs of relevant tests both with patched and non-patched JSC. Looks good.

Pactched version:
1.
element.expando = value:Runs -> [15259, 15286, 15789, 15963, 16001] runs/s
    mean: 15659.6 runs/s
    median: 15789 runs/s
    stdev: 362.42488877007344 runs/s
    min: 15259 runs/s
    max: 16001 runs/s

element.expando:Runs -> [177329, 293077, 294280, 294527, 295861] runs/s
    mean: 271014.8 runs/s
    median: 294280 runs/s
    stdev: 52381.28607814054 runs/s
    min: 177329 runs/s
    max: 295861 runs/s


:Runs -> [14050.009403493468, 14528.250866673368, 14985.00953013684, 15142.30571354955, 15180.021487067997] runs/s
    mean: 14777.119400184245 runs/s
    median: 14985.00953013684 runs/s
    stdev: 482.0617013090354 runs/s
    min: 14050.009403493468 runs/s
    max: 15180.021487067997 runs/s


2.
element.expando = value:Runs -> [15324, 15624, 15644, 15783, 15821] runs/s
    mean: 15639.2 runs/s
    median: 15644 runs/s
    stdev: 195.7899384544569 runs/s
    min: 15324 runs/s
    max: 15821 runs/s

element.expando:Runs -> [171449, 291956, 292678, 292877, 294543] runs/s
    mean: 268700.6 runs/s
    median: 292678 runs/s
    stdev: 54373.54825004526 runs/s
    min: 171449 runs/s
    max: 294543 runs/s


:Runs -> [14066.72525472097, 14830.354847519346, 14850.23654491084, 14975.953123177605, 15014.514579654855] runs/s
    mean: 14747.556869996724 runs/s
    median: 14850.23654491084 runs/s
    stdev: 388.7034783399407 runs/s
    min: 14066.72525472097 runs/s
    max: 15014.514579654855 runs/s



Unpatched version:
1.
element.expando = value:Runs -> [780.2197802197802, 797, 800, 802, 805] runs/s
    mean: 796.843956043956 runs/s
    median: 800 runs/s
    stdev: 9.739789875703048 runs/s
    min: 780.2197802197802 runs/s
    max: 805 runs/s

element.expando:Runs -> [1010, 1014, 1015, 1019, 1019] runs/s
    mean: 1015.4 runs/s
    median: 1015 runs/s
    stdev: 3.781534080237811 runs/s
    min: 1010 runs/s
    max: 1019 runs/s


:Runs -> [440.18169541464613, 446.24958586416346, 447.3829201101929, 448.78528281164193, 449.7231359649123] runs/s
    mean: 446.46452403311133 runs/s
    median: 447.3829201101929 runs/s
    stdev: 3.753933212314692 runs/s
    min: 440.18169541464613 runs/s
    max: 449.7231359649123 runs/s

2.
element.expando = value:Runs -> [763, 791, 793, 793.2067932067932, 797] runs/s
    mean: 787.4413586413586 runs/s
    median: 793 runs/s
    stdev: 13.83428977812362 runs/s
    min: 763 runs/s
    max: 797 runs/s

element.expando:Runs -> [1001, 1007, 1008, 1008, 1013] runs/s
    mean: 1007.4 runs/s
    median: 1008 runs/s
    stdev: 4.277849927241485 runs/s
    min: 1001 runs/s
    max: 1013 runs/s


:Runs -> [432.97222222222223, 443.01279199110115, 443.83342587451415, 443.8981967911402, 446.05580110497243] runs/s
    mean: 441.95448759679005 runs/s
    median: 443.83342587451415 runs/s
    stdev: 5.146121996860227 runs/s
    min: 432.97222222222223 runs/s
    max: 446.05580110497243 runs/s

[WebKit Commit Bot]

Comment on attachment 291215 [details]
patch

Clearing flags on attachment: 291215

Committed r207060: <http://trac.webkit.org/changeset/207060>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 291245 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 291245 [details]
Patch for landing

Rejecting attachment 291245 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 291245, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
/show_bug.cgi?id=163182&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 291245 from bug 163182.
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 255 cwd: /Volumes/Data/EWS/WebKit

No diff found. at /Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply line 128.
Parsed 0 diffs from patch file(s).

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 255 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/2261477

[youenn fablet]

(In reply to comment #10)
> Comment on attachment 291245 [details]
> Patch for landing

Wrong bug

[Darin Adler]

Comment on attachment 291215 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:345
> +                    if (!value)
> +                        return String();
> +                    if (value.isInt32())
> +                        return String::number(value.asInt32());
> +                    if (value.isNumber())
> +                        return String::numberToStringECMAScript(value.asNumber());
> +                    if (value.isBoolean())
> +                        return value.asBoolean() ? ASCIILiteral("true") : ASCIILiteral("false");
> +                    if (value.isNull())
> +                        return ASCIILiteral("null");
> +                    if (value.isUndefined())
> +                        return ASCIILiteral("undefined");

This seems so repetitive with code in the JSValue class. Is there some way to share more of this?

[Filip Pizlo]

(In reply to comment #12)
> Comment on attachment 291215 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291215&action=review
> 
> > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:345
> > +                    if (!value)
> > +                        return String();
> > +                    if (value.isInt32())
> > +                        return String::number(value.asInt32());
> > +                    if (value.isNumber())
> > +                        return String::numberToStringECMAScript(value.asNumber());
> > +                    if (value.isBoolean())
> > +                        return value.asBoolean() ? ASCIILiteral("true") : ASCIILiteral("false");
> > +                    if (value.isNull())
> > +                        return ASCIILiteral("null");
> > +                    if (value.isUndefined())
> > +                        return ASCIILiteral("undefined");
> 
> This seems so repetitive with code in the JSValue class. Is there some way
> to share more of this?

The cost of sharing is probably greater than the benefit.

This code is subtly different.  The JSValue code uses VM number string caches, which the JIT can't do.  The JIT gives up for effects while JSValue executes them.  It's repetitive in structure, but if you tried to build an abstraction that deduplicates the code then your abstraction is likely to be about the same amount of code than writing this twice.

We often have repetitive code like this in the JIT.  I think that it's served us well because the worst case is that the JIT constant folds according to wrong rules.  The worst case if you share code is that the shared code goes down a path that isn't legal in the JIT, like accessing some VM state that isn't protected by locks.  Constant folding rules are trivial to write tests for, so we're more paranoid about the races.

Finally, I think that this duplication is mostly harmless because it's just a statement of core JSC semantics.  Duplication sucks most if you want to change the duplicated code later.  But the stringification rules of these types can't be changed without breaking the web.

[Darin Adler]

Comment on attachment 291215 [details]
patch

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

>>> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:345
>>> +                        return ASCIILiteral("undefined");
>> 
>> This seems so repetitive with code in the JSValue class. Is there some way to share more of this?
> 
> The cost of sharing is probably greater than the benefit.
> 
> This code is subtly different.  The JSValue code uses VM number string caches, which the JIT can't do.  The JIT gives up for effects while JSValue executes them.  It's repetitive in structure, but if you tried to build an abstraction that deduplicates the code then your abstraction is likely to be about the same amount of code than writing this twice.
> 
> We often have repetitive code like this in the JIT.  I think that it's served us well because the worst case is that the JIT constant folds according to wrong rules.  The worst case if you share code is that the shared code goes down a path that isn't legal in the JIT, like accessing some VM state that isn't protected by locks.  Constant folding rules are trivial to write tests for, so we're more paranoid about the races.
> 
> Finally, I think that this duplication is mostly harmless because it's just a statement of core JSC semantics.  Duplication sucks most if you want to change the duplicated code later.  But the stringification rules of these types can't be changed without breaking the web.

Not surprising that the one used in actual runtime has some optimizations like that. I should have looked; I see now that there isn’t really good code to share with.

I wasn't suggesting we build try to build a significant new abstraction, so that’s a bit of a straw man.

I think my concern grew out of worries that this code might have used the wrong function, say String::number instead of String::numberToStringECMAScript, and it was likely we would not cover that in testing. I think we may need a couple more test cases for floating point that were included in the original patch.

[Filip Pizlo]

(In reply to comment #14)
> Comment on attachment 291215 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291215&action=review
> 
> >>> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:345
> >>> +                        return ASCIILiteral("undefined");
> >> 
> >> This seems so repetitive with code in the JSValue class. Is there some way to share more of this?
> > 
> > The cost of sharing is probably greater than the benefit.
> > 
> > This code is subtly different.  The JSValue code uses VM number string caches, which the JIT can't do.  The JIT gives up for effects while JSValue executes them.  It's repetitive in structure, but if you tried to build an abstraction that deduplicates the code then your abstraction is likely to be about the same amount of code than writing this twice.
> > 
> > We often have repetitive code like this in the JIT.  I think that it's served us well because the worst case is that the JIT constant folds according to wrong rules.  The worst case if you share code is that the shared code goes down a path that isn't legal in the JIT, like accessing some VM state that isn't protected by locks.  Constant folding rules are trivial to write tests for, so we're more paranoid about the races.
> > 
> > Finally, I think that this duplication is mostly harmless because it's just a statement of core JSC semantics.  Duplication sucks most if you want to change the duplicated code later.  But the stringification rules of these types can't be changed without breaking the web.
> 
> Not surprising that the one used in actual runtime has some optimizations
> like that. I should have looked; I see now that there isn’t really good code
> to share with.
> 
> I wasn't suggesting we build try to build a significant new abstraction, so
> that’s a bit of a straw man.

FWIW, we build such abstractions when we find that the code saved is bigger than the size of the abstraction.  Refactoring Yarr to be completely thread-safe so that the JIT can run it to constant-folding regular expression operations is one extreme example.  So, I didn't mean it as a straw man.  It probably would have been worth it if we were only abstracting whether to call the effectful slow path.

> 
> I think my concern grew out of worries that this code might have used the
> wrong function, say String::number instead of
> String::numberToStringECMAScript, and it was likely we would not cover that
> in testing. I think we may need a couple more test cases for floating point
> that were included in the original patch.

Isn't String::numberToStringECMAScript already the abstraction of how you convert non-int numbers to strings?

String::number is safe for int32's, in the sense that the rest of the VM also already assumes that this is a valid int32->string conversion according to ES.

[Saam Barati]

(In reply to comment #14)
> Comment on attachment 291215 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291215&action=review
> 
> >>> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:345
> >>> +                        return ASCIILiteral("undefined");
> >> 
> >> This seems so repetitive with code in the JSValue class. Is there some way to share more of this?
> > 
> > The cost of sharing is probably greater than the benefit.
> > 
> > This code is subtly different.  The JSValue code uses VM number string caches, which the JIT can't do.  The JIT gives up for effects while JSValue executes them.  It's repetitive in structure, but if you tried to build an abstraction that deduplicates the code then your abstraction is likely to be about the same amount of code than writing this twice.
> > 
> > We often have repetitive code like this in the JIT.  I think that it's served us well because the worst case is that the JIT constant folds according to wrong rules.  The worst case if you share code is that the shared code goes down a path that isn't legal in the JIT, like accessing some VM state that isn't protected by locks.  Constant folding rules are trivial to write tests for, so we're more paranoid about the races.
> > 
> > Finally, I think that this duplication is mostly harmless because it's just a statement of core JSC semantics.  Duplication sucks most if you want to change the duplicated code later.  But the stringification rules of these types can't be changed without breaking the web.
> 
> Not surprising that the one used in actual runtime has some optimizations
> like that. I should have looked; I see now that there isn’t really good code
> to share with.
> 
> I wasn't suggesting we build try to build a significant new abstraction, so
> that’s a bit of a straw man.
> 
> I think my concern grew out of worries that this code might have used the
> wrong function, say String::number instead of
> String::numberToStringECMAScript, and it was likely we would not cover that
> in testing. I think we may need a couple more test cases for floating point
> that were included in the original patch.
I agree that it's worth adding more floating point tests to prevent regressions in the future. An early version of my patch had a FIXME for the double case because I was tempted to use String::number but was suspicious of its correctness (this is before I looked at what the runtime version did). So I can see how this code could go wrong. I'll do the tests in a follow up patch.

[Saam Barati]

Bug for tests:
https://bugs.webkit.org/show_bug.cgi?id=163517