The bytecode generator does not currently know how to inline Reflect.apply (see https://bugs.webkit.org/show_bug.cgi?id=190668). Hence, it's a waste of time to emit the ApplyFunctionCallDotNode since the function check against Function.apply that it will generate will always fail. It also causes DFG graph dumping to crash. <rdar://problem/45201145>
Created attachment 352592 [details] proposed patch. Let's get some EWS testing first.
Comment on attachment 352592 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=352592&action=review > Source/JavaScriptCore/ChangeLog:12 > + that it will generate will always fail. It can also cause DFG graph dumping to I agree with this from a performance perspective. However, it seems like you’re sidestepping a crash. Why does the dumper crash? Seems like that should be fixed
(In reply to Saam Barati from comment #2) > Comment on attachment 352592 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=352592&action=review > > > Source/JavaScriptCore/ChangeLog:12 > > + that it will generate will always fail. It can also cause DFG graph dumping to > > I agree with this from a performance perspective. However, it seems like > you’re sidestepping a crash. Why does the dumper crash? Seems like that > should be fixed It crashed because it's expecting the callee in a CallVariant to be a function or executable of some form. Because we're creating a ApplyFunctionCallDotNode for Reflect.apply, we end up passing the Reflect object as the callee. This is wrong, and is the reason the dumper crashed. We should be passing the first argument of Reflect.apply as the callee instead (when we support inlining Reflect.apply). Hmmm ... that said, I haven't explored what happens in we can properly inline Reflect.apply later and pass a non-function value as the first argument. I think we can leave that as an exercise for https://bugs.webkit.org/show_bug.cgi?id=190668.
Comment on attachment 352592 [details] proposed patch. Attachment 352592 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9638635 Number of test failures exceeded the failure limit.
Created attachment 352601 [details] Archive of layout-test-results from ews100 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 352592 [details] proposed patch. Attachment 352592 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9638736 Number of test failures exceeded the failure limit.
Created attachment 352602 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
(In reply to Mark Lam from comment #3) > (In reply to Saam Barati from comment #2) > > Comment on attachment 352592 [details] > > proposed patch. > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=352592&action=review > > > > > Source/JavaScriptCore/ChangeLog:12 > > > + that it will generate will always fail. It can also cause DFG graph dumping to > > > > I agree with this from a performance perspective. However, it seems like > > you’re sidestepping a crash. Why does the dumper crash? Seems like that > > should be fixed > > It crashed because it's expecting the callee in a CallVariant to be a > function or executable of some form. Because we're creating a > ApplyFunctionCallDotNode for Reflect.apply, we end up passing the Reflect > object as the callee. This is wrong, and is the reason the dumper crashed. > We should be passing the first argument of Reflect.apply as the callee > instead (when we support inlining Reflect.apply). I don't see how this matters at all that the callee here is Reflect. Seems like any code can trigger this. > > Hmmm ... that said, I haven't explored what happens in we can properly > inline Reflect.apply later and pass a non-function value as the first > argument. I think we can leave that as an exercise for > https://bugs.webkit.org/show_bug.cgi?id=190668.
Comment on attachment 352592 [details] proposed patch. Attachment 352592 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9638622 Number of test failures exceeded the failure limit.
Created attachment 352605 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
(In reply to Saam Barati from comment #8) > I don't see how this matters at all that the callee here is Reflect. Seems > like any code can trigger this. You are right. I stand corrected. Will fix.
Created attachment 352635 [details] proposed patch. Let's try on the EWS again.
Comment on attachment 352635 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=352635&action=review r=me > Source/JavaScriptCore/ChangeLog:11 > + https://bugs.webkit.org/show_bug.cgi?id=190668). Hence, it's a waste of time to > + emit the ApplyFunctionCallDotNode since the function check against Function.apply What you're doing is a heuristic, FWIW. I agree with your change, because it's probably better, but it's still a heuristic. A user could write code where this new heuristic is worse: ``` function Reflect() { ... } Reflect.apply ```
Thanks for the review. (In reply to Saam Barati from comment #13) > > Source/JavaScriptCore/ChangeLog:11 > > + https://bugs.webkit.org/show_bug.cgi?id=190668). Hence, it's a waste of time to > > + emit the ApplyFunctionCallDotNode since the function check against Function.apply > > What you're doing is a heuristic, FWIW. I agree with your change, because > it's probably better, but it's still a heuristic. A user could write code > where this new heuristic is worse: > ``` > function Reflect() { ... } > Reflect.apply > ``` Yes, I thought of that. My thinking is that if the user wrote code like that, he/she is just asking for it. For most normal code, I would not expect the user to create a function named Reflect with a member named apply. So, I opted to go with this heuristic.
Comment on attachment 352635 [details] proposed patch. Clearing flags on attachment: 352635 Committed r237241: <https://trac.webkit.org/changeset/237241>
All reviewed patches have been landed. Closing bug.