Bug 63409 - DFG JIT does not perform put_by_id caching
Summary: DFG JIT does not perform put_by_id caching
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-26 14:07 PDT by Filip Pizlo
Modified: 2011-06-27 14:38 PDT (History)
2 users (show)

See Also:


Attachments
the patch (21.57 KB, patch)
2011-06-26 14:31 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (fix style) (21.42 KB, patch)
2011-06-26 16:15 PDT, Filip Pizlo
barraclough: review-
Details | Formatted Diff | Diff
the patch (fix review) (22.50 KB, patch)
2011-06-27 14:15 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (fix review) (21.95 KB, patch)
2011-06-27 14:17 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2011-06-26 14:07:15 PDT
The main JSC JIT performs extensive caching optimizations for put_by_id.  The DFG JIT currently always emits a call to a helper function, which is implemented in C.  At least the basic put_by_id caching (such as replacement of existing properties) should be ported to DFG.
Comment 1 Filip Pizlo 2011-06-26 14:31:05 PDT
Created attachment 98639 [details]
the patch
Comment 2 WebKit Review Bot 2011-06-26 14:34:07 PDT
Attachment 98639 [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/DFGOperations.cpp:288:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGOperations.cpp:288:  The parameter name "returnAddress" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGOperations.cpp:305:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGOperations.cpp:305:  The parameter name "returnAddress" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGOperations.cpp:322:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGOperations.cpp:322:  The parameter name "returnAddress" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGOperations.cpp:339:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGOperations.cpp:339:  The parameter name "returnAddress" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGRepatch.cpp:53:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/JavaScriptCore/dfg/DFGRepatch.cpp:55:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/JavaScriptCore/dfg/DFGRepatch.cpp:102:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/JavaScriptCore/dfg/DFGRepatch.cpp:107:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 12 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Filip Pizlo 2011-06-26 16:15:50 PDT
Created attachment 98642 [details]
the patch (fix style)
Comment 4 Gavin Barraclough 2011-06-27 12:00:56 PDT
Comment on attachment 98642 [details]
the patch (fix style)

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

Hey Filip, this patch looks great, and I'd be happy to accept as is.  I've r-ed to bounce back to you to give you a chance to look at a couple of suggestions, but feel free to consider both optional, and just reset r? on this patch if you want.  Our coding style leans against bool parameters where possible, since their meaning at the call site is often opaque, in many cases we prefer enumerated types.  In this case I'm not sure that I'd recommend adding an extra enum, so I'd either pass a function pointer or stick with the bool.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:366
> +    if (m_jit.codeBlock()->isStrictMode()) {

Given that 'direct' is only used to select the operation to call, I think it might make the code a little clearer to make this function take a V_DFGOperation_EJJI rather than a bool.  It would remove this chunk of code, and make the call site a little more transparent (the meaning of bool arguments isn't always obvious).  Obviously each call site would still need to do something like "m_jit.codeBlock()->isStrictMode() ? operationPutByIdDirectStrictOptimize : operationPutByIdDirectNonStrictOptimize".  What do you think?

> Source/JavaScriptCore/dfg/DFGRepatch.cpp:138
> +void dfgRepatchPutByID(ExecState* exec, JSValue baseValue, const Identifier& propertyName, const PutPropertySlot& slot, StructureStubInfo& stubInfo, bool direct)

Similarly to during compilation, again you could switch the bool argument to a V_DFGOperation_EJJI, and ditch the appropriatePutByIdFunction method.
Comment 5 Filip Pizlo 2011-06-27 12:12:28 PDT
(In reply to comment #4)
> (From update of attachment 98642 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98642&action=review
> 
> Hey Filip, this patch looks great, and I'd be happy to accept as is.  I've r-ed to bounce back to you to give you a chance to look at a couple of suggestions, but feel free to consider both optional, and just reset r? on this patch if you want.  Our coding style leans against bool parameters where possible, since their meaning at the call site is often opaque, in many cases we prefer enumerated types.  In this case I'm not sure that I'd recommend adding an extra enum, so I'd either pass a function pointer or stick with the bool.

I totally agree that bool is nasty.  We already have this bool thing for direct in other places (PutPropertySlot mainly) so I followed that pattern.  But I don't particularly like it.

I'm not entirely sure I like your suggestion either; I like the notion that dfgRepatchPutByID and friends abstract away what slow-path calls they're making, as much as possible.

What about this: add an enumeration in PutPropertySlot (or some other appropriate place):

enum PutKind { Direct, NotDirect };

And replace the "bool direct" throughout with this enum?  I'd be happy to make this refactoring as part of this patch.
Comment 6 Gavin Barraclough 2011-06-27 12:18:44 PDT
> What about this: add an enumeration in PutPropertySlot (or some other appropriate place):
> 
> enum PutKind { Direct, NotDirect };

Yep, sounds great, that works for me.
Comment 7 Filip Pizlo 2011-06-27 14:15:05 PDT
Created attachment 98783 [details]
the patch (fix review)
Comment 8 Filip Pizlo 2011-06-27 14:16:09 PDT
(In reply to comment #7)
> Created an attachment (id=98783) [details]
> the patch (fix review)

Ahhh!  I'll back this one out, since it has the change to Platform.h

-F
Comment 9 Filip Pizlo 2011-06-27 14:17:07 PDT
Created attachment 98784 [details]
the patch (fix review)
Comment 10 Gavin Barraclough 2011-06-27 14:28:51 PDT
Comment on attachment 98784 [details]
the patch (fix review)

Looks great
Comment 11 WebKit Review Bot 2011-06-27 14:38:21 PDT
Comment on attachment 98784 [details]
the patch (fix review)

Clearing flags on attachment: 98784

Committed r89861: <http://trac.webkit.org/changeset/89861>
Comment 12 WebKit Review Bot 2011-06-27 14:38:26 PDT
All reviewed patches have been landed.  Closing bug.