Bug 85721

Summary: DFG should support reflective arguments access
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
work in progress
none
more
none
the patch
none
the patch oliver: review+

Description Filip Pizlo 2012-05-05 14:32:17 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2012-05-05 14:33:57 PDT
Created attachment 140410 [details]
work in progress
Comment 2 Filip Pizlo 2012-05-05 15:42:58 PDT
Created attachment 140411 [details]
more

Added support for op_create_arguments.  Still more work to do though.
Comment 3 Filip Pizlo 2012-05-05 17:50:11 PDT
Created attachment 140415 [details]
the patch
Comment 4 Sam Weinig 2012-05-05 18:59:35 PDT
Comment on attachment 140415 [details]
the patch

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

> Source/JavaScriptCore/runtime/Arguments.h:53
> -        bool overrodeLength : 1;
> +        bool overrodeLength; // make this a full byte to make it easy to test from the JIT.
>          bool overrodeCallee : 1;
>          bool overrodeCaller : 1;
>          bool isStrictMode : 1;

Is keeping the rest of these bit fields actually saving space?
Comment 5 Filip Pizlo 2012-05-05 19:25:31 PDT
(In reply to comment #4)
> (From update of attachment 140415 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140415&action=review
> 
> > Source/JavaScriptCore/runtime/Arguments.h:53
> > -        bool overrodeLength : 1;
> > +        bool overrodeLength; // make this a full byte to make it easy to test from the JIT.
> >          bool overrodeCallee : 1;
> >          bool overrodeCaller : 1;
> >          bool isStrictMode : 1;
> 
> Is keeping the rest of these bit fields actually saving space?

Good point. I will change.
Comment 6 Oliver Hunt 2012-05-06 11:59:47 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 140415 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=140415&action=review
> > 
> > > Source/JavaScriptCore/runtime/Arguments.h:53
> > > -        bool overrodeLength : 1;
> > > +        bool overrodeLength; // make this a full byte to make it easy to test from the JIT.
> > >          bool overrodeCallee : 1;
> > >          bool overrodeCaller : 1;
> > >          bool isStrictMode : 1;
> > 
> > Is keeping the rest of these bit fields actually saving space?
> 
> Good point. I will change.

It occurs to me that the dfg uses a structure check for arguments -- what happens if we made overriding the length change the structure -> then we'd just have the single structure check and all would be well.  Not suggesting that change for this patch though.
Comment 7 Filip Pizlo 2012-05-06 12:07:34 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 140415 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=140415&action=review
> > > 
> > > > Source/JavaScriptCore/runtime/Arguments.h:53
> > > > -        bool overrodeLength : 1;
> > > > +        bool overrodeLength; // make this a full byte to make it easy to test from the JIT.
> > > >          bool overrodeCallee : 1;
> > > >          bool overrodeCaller : 1;
> > > >          bool isStrictMode : 1;
> > > 
> > > Is keeping the rest of these bit fields actually saving space?
> > 
> > Good point. I will change.
> 
> It occurs to me that the dfg uses a structure check for arguments -- what happens if we made overriding the length change the structure -> then we'd just have the single structure check and all would be well.  Not suggesting that change for this patch though.

We could do that.

Or we could have the CFA infer that you're accessing your own arguments, and then we don't even have to go down the code path that accesses the Arguments object.  That's my next project.

This entire GetByVal/PutByVal code path is there to ensure that even if the CFA fails to prove whatever we want it to prove, we still run at OK speed. I wouldn't want to put in real effort to optimize this fall-back path beyond what's there in this patch.
Comment 8 Filip Pizlo 2012-05-06 21:22:36 PDT
Created attachment 140469 [details]
the patch

Rebased patch with Sam's suggestions.
Comment 9 Oliver Hunt 2012-05-06 22:44:27 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > (From update of attachment 140415 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=140415&action=review
> > > > 
> > > > > Source/JavaScriptCore/runtime/Arguments.h:53
> > > > > -        bool overrodeLength : 1;
> > > > > +        bool overrodeLength; // make this a full byte to make it easy to test from the JIT.
> > > > >          bool overrodeCallee : 1;
> > > > >          bool overrodeCaller : 1;
> > > > >          bool isStrictMode : 1;
> > > > 
> > > > Is keeping the rest of these bit fields actually saving space?
> > > 
> > > Good point. I will change.
> > 
> > It occurs to me that the dfg uses a structure check for arguments -- what happens if we made overriding the length change the structure -> then we'd just have the single structure check and all would be well.  Not suggesting that change for this patch though.
> 
> We could do that.
> 
> Or we could have the CFA infer that you're accessing your own arguments, and then we don't even have to go down the code path that accesses the Arguments object.  That's my next project.

Although you'll still need to do deleted value checks i think if the arguments object is present.

> 
> This entire GetByVal/PutByVal code path is there to ensure that even if the CFA fails to prove whatever we want it to prove, we still run at OK speed. I wouldn't want to put in real effort to optimize this fall-back path beyond what's there in this patch.

Fair enough.  r=me
Comment 10 Filip Pizlo 2012-05-06 22:48:09 PDT
(In reply to comment #9)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > (In reply to comment #4)
> > > > > (From update of attachment 140415 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=140415&action=review
> > > > > 
> > > > > > Source/JavaScriptCore/runtime/Arguments.h:53
> > > > > > -        bool overrodeLength : 1;
> > > > > > +        bool overrodeLength; // make this a full byte to make it easy to test from the JIT.
> > > > > >          bool overrodeCallee : 1;
> > > > > >          bool overrodeCaller : 1;
> > > > > >          bool isStrictMode : 1;
> > > > > 
> > > > > Is keeping the rest of these bit fields actually saving space?
> > > > 
> > > > Good point. I will change.
> > > 
> > > It occurs to me that the dfg uses a structure check for arguments -- what happens if we made overriding the length change the structure -> then we'd just have the single structure check and all would be well.  Not suggesting that change for this patch though.
> > 
> > We could do that.
> > 
> > Or we could have the CFA infer that you're accessing your own arguments, and then we don't even have to go down the code path that accesses the Arguments object.  That's my next project.
> 
> Although you'll still need to do deleted value checks i think if the arguments object is present.

But that ought to be my slow path, right?

> 
> > 
> > This entire GetByVal/PutByVal code path is there to ensure that even if the CFA fails to prove whatever we want it to prove, we still run at OK speed. I wouldn't want to put in real effort to optimize this fall-back path beyond what's there in this patch.
> 
> Fair enough.  r=me

Thanks!
Comment 11 Oliver Hunt 2012-05-07 09:37:22 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > (In reply to comment #5)
> > > > > (In reply to comment #4)
> > > > > > (From update of attachment 140415 [details] [details] [details] [details] [details] [details])
> > > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=140415&action=review
> > > > > > 
> > > > > > > Source/JavaScriptCore/runtime/Arguments.h:53
> > > > > > > -        bool overrodeLength : 1;
> > > > > > > +        bool overrodeLength; // make this a full byte to make it easy to test from the JIT.
> > > > > > >          bool overrodeCallee : 1;
> > > > > > >          bool overrodeCaller : 1;
> > > > > > >          bool isStrictMode : 1;
> > > > > > 
> > > > > > Is keeping the rest of these bit fields actually saving space?
> > > > > 
> > > > > Good point. I will change.
> > > > 
> > > > It occurs to me that the dfg uses a structure check for arguments -- what happens if we made overriding the length change the structure -> then we'd just have the single structure check and all would be well.  Not suggesting that change for this patch though.
> > > 
> > > We could do that.
> > > 
> > > Or we could have the CFA infer that you're accessing your own arguments, and then we don't even have to go down the code path that accesses the Arguments object.  That's my next project.
> > 
> > Although you'll still need to do deleted value checks i think if the arguments object is present.
> 
> But that ought to be my slow path, right?

Sorry, it sounded like you were planning on doing inline property access even for reified arguments objects.

The more i think about it though, the more i like the idea of having this info marked off by a structure check, it would allow us to do sane things even when the arguments have been reified.
Comment 12 Filip Pizlo 2012-05-07 13:19:26 PDT
Landed in http://trac.webkit.org/changeset/116345
Comment 13 Filip Pizlo 2012-05-22 13:01:59 PDT
Merged in http://trac.webkit.org/changeset/118030