Bug 85721 – DFG should support reflective arguments access

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 140410 [details]
work in progress

[Filip Pizlo]

Created attachment 140411 [details]
more

Added support for op_create_arguments.  Still more work to do though.

[Filip Pizlo]

Created attachment 140415 [details]
the patch

[Sam Weinig]

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?

[Filip Pizlo]

(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.

[Oliver Hunt]

(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.

[Filip Pizlo]

(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.

[Filip Pizlo]

Created attachment 140469 [details]
the patch

Rebased patch with Sam's suggestions.

[Oliver Hunt]

(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

[Filip Pizlo]

(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!

[Oliver Hunt]

(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.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/116345

[Filip Pizlo]

Merged in http://trac.webkit.org/changeset/118030