Bug 63173 – DFG non-speculative JIT lacks basic optimizations for GetById and GetByVal

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

[Filip Pizlo]

The main JSC JIT has optimizations to speed up GetById and GetByVal for common cases.  The DFG speculative JIT has some of these optimizations as well.  But the non-speculative JIT always calls out to C, potentially resulting in performance regressions on code that fails speculation.

[Filip Pizlo]

Created attachment 98226 [details]
the patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:328:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.cpp:502:  Missing spaces around =  [whitespace/operators] [4]
Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.cpp:503:  Missing spaces around !=  [whitespace/operators] [3]
Total errors found: 4 in 5 files


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

[Filip Pizlo]

Created attachment 98230 [details]
the patch (fixed style)

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 5 files


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

[Filip Pizlo]

Created attachment 98231 [details]
the patch (more style fixes)

[Gavin Barraclough]

Comment on attachment 98231 [details]
the patch (more style fixes)

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

Please fix comments above, as discussed!

> Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.cpp:502
> +        NodeIndex alias = node.child3;

The alias optimization is unsafe on the non-speculative path.
This assumes that between subsequent GetByVals to the same location, the value / array has not changed.  We ensure this is true on the speculative path by restricting generation of operations that would trigger side-effects, but do not do so on the non-speculative path.

> Source/JavaScriptCore/dfg/DFGNonSpeculativeJIT.cpp:586
> +        cachedGetById(baseGPR, resultGPR, node.identifierNumber());

The code generation for this method is going to generate two calls, please refactor to make one call.

[Filip Pizlo]

Created attachment 98280 [details]
the patch (fixed review comments)

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:335:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 5 files


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

[Filip Pizlo]

Created attachment 98300 [details]
the patch (fixed review, fixed style)

[Gavin Barraclough]

Comment on attachment 98300 [details]
the patch (fixed review, fixed style)

Hey Filip, sorry to be picky but Darin will be disappointed in me if I'm not. :-)
WebKit coding style calls for full names, bar cases of very commonly idiomatic abbreviations.  really 'slowPathTarg' has to be 'slowPathTarget'.  Sorry to bounce your patch for two extra letter, but we should do this right.

[Filip Pizlo]

Created attachment 98411 [details]
the patch (fix identifier name)

[Gavin Barraclough]

Comment on attachment 98411 [details]
the patch (fix identifier name)

Looks great Filip!

[WebKit Review Bot]

Comment on attachment 98411 [details]
the patch (fix identifier name)

Clearing flags on attachment: 98411

Committed r89643: <http://trac.webkit.org/changeset/89643>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.