Patch forthcoming.
Created attachment 140410 [details] work in progress
Created attachment 140411 [details] more Added support for op_create_arguments. Still more work to do though.
Created attachment 140415 [details] the patch
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?
(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.
(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.
(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.
Created attachment 140469 [details] the patch Rebased patch with Sam's suggestions.
(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
(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!
(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.
Landed in http://trac.webkit.org/changeset/116345
Merged in http://trac.webkit.org/changeset/118030