Bug 200480

Summary: [JSC] Add "jump if (not) undefined or null" bytecode ops
Product: WebKit Reporter: Ross Kirsling <ross.kirsling>
Component: JavaScriptCoreAssignee: Ross Kirsling <ross.kirsling>
Status: RESOLVED FIXED    
Severity: Normal CC: chi187, commit-queue, darin, ews-watchlist, ggaren, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=200199
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Ross Kirsling
Reported 2019-08-06 15:41:42 PDT
[JSC] Add "jump if (not) undefined or null" bytecode ops
Attachments
Patch (16.70 KB, patch)
2019-08-06 15:44 PDT, Ross Kirsling
no flags
Patch (17.21 KB, patch)
2019-08-06 22:45 PDT, Ross Kirsling
no flags
Patch (18.76 KB, patch)
2019-08-07 14:22 PDT, Ross Kirsling
no flags
Patch for landing (18.61 KB, patch)
2019-08-08 10:49 PDT, Ross Kirsling
no flags
Ross Kirsling
Comment 1 2019-08-06 15:44:03 PDT
Ross Kirsling
Comment 2 2019-08-06 15:49:51 PDT
I did this as an optimization to follow bug 200199, but it turns out that: - this patch doesn't actually conflict with that one - adding this functionality simultaneously addresses an existing FIXME in BytecodeGenerator :D
Ross Kirsling
Comment 3 2019-08-06 15:59:08 PDT
Hmm, evidently that FIXME was intentionally added instead of creating new ops back then (see bug 151596 comment 6, say). However, it's four years later and the situation is drastically different: - we have a new bytecode format now (r237547) - we already have OpIsUndefinedOrNull (r239761) - we have major use cases for these fused jumps with ?? and ?. landing (r247819, bug 200199)
Darin Adler
Comment 4 2019-08-06 19:42:35 PDT
While I would love to review this patch, given the history I think it should be Geoff or one of the people currently actively working on the JavaScript engineer. Geoff?
Saam Barati
Comment 5 2019-08-06 21:02:56 PDT
Comment on attachment 375656 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375656&action=review Seems sensible. Just one question before I r+ > Source/JavaScriptCore/ChangeLog:7 > + You should say why you’re doing this. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1537 > + } else if (m_lastOpcodeID == op_is_undefined_or_null && target.isForward()) { Do these have the same semantics for document.all?
Ross Kirsling
Comment 6 2019-08-06 21:17:12 PDT
(In reply to Saam Barati from comment #5) > Comment on attachment 375656 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=375656&action=review > > Seems sensible. Just one question before I r+ > > > Source/JavaScriptCore/ChangeLog:7 > > + > > You should say why you’re doing this. Will do. > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1537 > > + } else if (m_lastOpcodeID == op_is_undefined_or_null && target.isForward()) { > > Do these have the same semantics for document.all? Yeah, that's the motivation across the board here -- Yusuke introduced op_is_undefined_or_null to address a document.all-related bug in ArraySpeciesCreate and this patch on its own addresses one in RequireObjectCoercible, but my real concern is that ?? and ?. both treat document.all as an object and thus their functionality mostly boils down to op_jnundefined_or_null and op_jundefined_or_null, respectively.
Ross Kirsling
Comment 7 2019-08-06 22:45:19 PDT
Ross Kirsling
Comment 8 2019-08-06 22:46:17 PDT
Comment on attachment 375688 [details] Patch Filled in ChangeLog.
Geoffrey Garen
Comment 9 2019-08-07 11:21:33 PDT
I think enough has changed over the years that adding an opcode in this case is probably the right choice. I would like to see more tests, including constant arguments, null, undefined, and other truth-y and false-y values. Are we sure it's correct for optional chaining to treat document.all as an object? That seems to defeat the purpose of MasqueradesAsUndefined, since it reveals the object-ness of the value to the JS programmer in a direct way. (Not sure this is super important anymore, since document.all is mostly a legacy feature, and optional chaining is a new feature. But it would be good to get our story straight here.)
Ross Kirsling
Comment 10 2019-08-07 11:53:57 PDT
(In reply to Geoffrey Garen from comment #9) > I would like to see more tests, including constant arguments, null, > undefined, and other truth-y and false-y values. I've aimed to cover these cases in this test file as well as the one in bug 200199: https://github.com/WebKit/webkit/blob/master/JSTests/stress/nullish-coalescing.js But none of them are JIT-oriented right now so I should correct that. > Are we sure it's correct for optional chaining to treat document.all as an > object? That seems to defeat the purpose of MasqueradesAsUndefined, since it > reveals the object-ness of the value to the JS programmer in a direct way. > (Not sure this is super important anymore, since document.all is mostly a > legacy feature, and optional chaining is a new feature. But it would be good > to get our story straight here.) I had expressed a similar worry as a spec reviewer for the feature, but the counterarguments were: 1. Access/call chains with ?. ought to behave like they do without ?. when nothing nullish is encountered, i.e. document.all?.length should do the same as document.all.length. 2. WHATWG has expressed the goal of "masquerading as undefined" as, effectively, to prevent feature checks for document.all while preserving its functionality, but we would not expect newly-written code to intentionally feature-check document.all, while it would be reasonable for a function which takes an Array-like parameter and uses ?. or ?? to work even if document.all is passed. If you're really curious, there's more discussion at https://github.com/tc39/proposal-optional-chaining/issues/108.
Ross Kirsling
Comment 11 2019-08-07 14:22:18 PDT
Ross Kirsling
Comment 12 2019-08-07 14:25:18 PDT
Comment on attachment 375753 [details] Patch Made some ?? tests JIT-oriented (and will do similarly for the ?. patch). Please let me know if I ought to go even further in some fashion.
Darin Adler
Comment 13 2019-08-07 14:44:10 PDT
Comment on attachment 375753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375753&action=review > JSTests/stress/nullish-coalescing.js:25 > +for (let i = 0; i < 1e5; i++) { I’m not sure whether this idiom of doing things in a loop to make sure the gets JIT compiled is the standard way of making tests in JSTests/stress check JIT correctness. But if it’s not 100% standard then I suggest adding a brief comment explaining the technique and why 1e5 is a big enough number.
Geoffrey Garen
Comment 14 2019-08-07 14:58:45 PDT
> 1. Access/call chains with ?. ought to behave like they do without ?. when > nothing nullish is encountered, i.e. document.all?.length should do the same > as document.all.length. I guess I agree with this logic enough not to argue with it. :P
Ross Kirsling
Comment 15 2019-08-07 15:01:27 PDT
(In reply to Darin Adler from comment #13) > Comment on attachment 375753 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=375753&action=review > > > JSTests/stress/nullish-coalescing.js:25 > > +for (let i = 0; i < 1e5; i++) { > > I’m not sure whether this idiom of doing things in a loop to make sure the > gets JIT compiled is the standard way of making tests in JSTests/stress > check JIT correctness. But if it’s not 100% standard then I suggest adding a > brief comment explaining the technique and why 1e5 is a big enough number. This is purely mimicry on my part, since I took this to be a ubiquitous idiom in JSTests/stress -- in particular we have: 55 files using `< 1e3` 221 files using `< 1e4` 101 files using `< 1e5` 89 files using `< 1e6` 2 files using `< 1e7` 141 files using `< 1000` 578 files using `< 10000` 339 files using `< 100000` 61 files using `< 1000000` 6 files using `< 10000000` Otherwise I have no commitment to 1e5 whatsoever.
Saam Barati
Comment 16 2019-08-08 02:06:28 PDT
Comment on attachment 375753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375753&action=review r=me >>> JSTests/stress/nullish-coalescing.js:25 >>> +for (let i = 0; i < 1e5; i++) { >> >> I’m not sure whether this idiom of doing things in a loop to make sure the gets JIT compiled is the standard way of making tests in JSTests/stress check JIT correctness. But if it’s not 100% standard then I suggest adding a brief comment explaining the technique and why 1e5 is a big enough number. > > This is purely mimicry on my part, since I took this to be a ubiquitous idiom in JSTests/stress -- in particular we have: > > 55 files using `< 1e3` > 221 files using `< 1e4` > 101 files using `< 1e5` > 89 files using `< 1e6` > 2 files using `< 1e7` > > 141 files using `< 1000` > 578 files using `< 10000` > 339 files using `< 100000` > 61 files using `< 1000000` > 6 files using `< 10000000` > > Otherwise I have no commitment to 1e5 whatsoever. Yeah this is a sensible number for JSC. We run each test in various JIT tier up modes, so this will end up stressing all the tiers. However, if I have one nit, I’d change the body of the loop to a function the loop calls. (That way if we somehow break loop OSR entry we will still test the optimizing JITs.)
Ross Kirsling
Comment 17 2019-08-08 10:49:42 PDT
Created attachment 375819 [details] Patch for landing
WebKit Commit Bot
Comment 18 2019-08-08 11:23:25 PDT
Comment on attachment 375819 [details] Patch for landing Clearing flags on attachment: 375819 Committed r248426: <https://trac.webkit.org/changeset/248426>
WebKit Commit Bot
Comment 19 2019-08-08 11:23:27 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 20 2019-08-08 11:24:21 PDT
Note You need to log in before you can comment on or make changes to this bug.