Bug 200480 – [JSC] Add "jump if (not) undefined or null" bytecode ops

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Ross Kirsling]

[JSC] Add "jump if (not) undefined or null" bytecode ops

[Ross Kirsling]

Created attachment 375656 [details]
Patch

[Ross Kirsling]

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]

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]

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 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]

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

Created attachment 375688 [details]
Patch

[Ross Kirsling]

Comment on attachment 375688 [details]
Patch

Filled in ChangeLog.

[Geoffrey Garen]

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]

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

Created attachment 375753 [details]
Patch

[Ross Kirsling]

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 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]

> 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]

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

Created attachment 375819 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 375819 [details]
Patch for landing

Clearing flags on attachment: 375819

Committed r248426: <https://trac.webkit.org/changeset/248426>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54088357>