Bug 214525 – Remove OpIsObjectOrNull from ClassExprNode::emitBytecode()

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

[Alexey Shvayka]

Remove OpIsObjectOrNull from ClassExprNode::emitBytecode()

[Alexey Shvayka]

Created attachment 404649 [details]
Patch

[Darin Adler]

Comment on attachment 404649 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404649&action=review

Would like to review this but I am not necessarily a good person to make the call about the new terminology.

> Source/JavaScriptCore/ChangeLog:13
> +        1. Replaces OpIsObjectOrNull in ClassExprNode::emitBytecode() with emitIsObject() +
> +           emitIsNull(), fixing `document.all` as `superclass.prototype` case with DFG/FTL,
> +           and tweaks error message to reflect that `null` is allowed. Also removes
> +           `superclass === undefined` check as it's on a slow path and the spec [1] lacks it.

I suggest doing this as a separate patch. It’s a behavior change and resolves a potential incompatibility with other engines, I guess?

> Source/JavaScriptCore/ChangeLog:22
> +        2. Renames is_object_or_null bytecode op to typeof_is_object, fixing confusing
> +           operationObjectIsObject() name, and aligning it with typeof_is_undefined. New name
> +           offers better semantics and clearly communicates the op should be avoided when
> +           implementing new features because of `typeof` behavior with [[IsHTMLDDA]] objects [2].
> +
> +        3. Leveraging fast path of JSCell::isCallable(), aligns jsTypeofIsObject() with DFG
> +           inliner and compiler. No behavior change, microbenchmarks/is-object-or-null-*.js
> +           are neutral.

These seem like great ideas, but not necessarily connected with the above. Just inspired by it. Internal clarifications with no behavior change. Any performance benefit?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:-1504
> -                        // FIXME: This could just call getCallData.
> -                        // https://bugs.webkit.org/show_bug.cgi?id=144457

Should we close this bug? If I understand correctly, it implies there is a small performance benefit possible due to this refactoring.

[Saam Barati]

Comment on attachment 404649 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404649&action=review

>> Source/JavaScriptCore/ChangeLog:22
>> +           are neutral.
> 
> These seem like great ideas, but not necessarily connected with the above. Just inspired by it. Internal clarifications with no behavior change. Any performance benefit?

I think it might make sense to do this in a followup, and to also fix the second reference to the FIXME.

I also commented on a refactor that would be nice to do at the same time, where it makes various inlinings of the C++ code all call a single helper function.)

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:-1504
>> -                        // https://bugs.webkit.org/show_bug.cgi?id=144457
> 
> Should we close this bug? If I understand correctly, it implies there is a small performance benefit possible due to this refactoring.

We should probably just change the below to also call isCallable

(I think that perhaps this enhancement can be done in a separate patch?)

> Source/JavaScriptCore/runtime/Operations.cpp:109
> +bool jsTypeofIsObject(JSGlobalObject* globalObject, JSValue v)

Suggestion:

We have this implemented in a few different places. What I'd do is:
1. Move this to a header where it's ALWAYS_INLINE
2. Make AI call this
3. make operationTypeOfIsObject call it (You could even break this down into two functions, one that takes JSValue, one that takes JSObject*.)

[Alexey Shvayka]

Created attachment 404924 [details]
Patch

Bring back `superclass === undefined` check, revert AbstractInterpreter and jsTypeofIsObject changes.

[Alexey Shvayka]

(In reply to Darin Adler from comment #2)

Thank you for taking a look!

> I suggest doing this as a separate patch. It’s a behavior change and
> resolves a potential incompatibility with other engines, I guess?

It's unobservable: instead of directly throwing for `undefined`, `isConstructor` is called with it and also throws.
I believe it is better to remove 1 bytecode from ClassExprNode as `class extends undefined` is very rare.

> These seem like great ideas, but not necessarily connected with the above.
> Just inspired by it. Internal clarifications with no behavior change. Any
> performance benefit?

No performance benefit: inlining by `child.m_type` catches most of the cases.

> Suggestion:
> 
> We have this implemented in a few different places. What I'd do is:
> 1. Move this to a header where it's ALWAYS_INLINE
> 2. Make AI call this
> 3. make operationTypeOfIsObject call it (You could even break this down into
> two functions, one that takes JSValue, one that takes JSObject*.)

That's a great idea! Thank you, Saam.
Let's defer this to a follow-up.

[Radar WebKit Bug Importer]

<rdar://problem/66110833>

[Keith Miller]

Comment on attachment 404924 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404924&action=review

r=me with some code relocation.

> Source/JavaScriptCore/ChangeLog:12
> +        1. Replaces OpIsObjectOrNull in ClassExprNode::emitBytecode() with emitIsObject() +
> +           emitIsNull(), fixing `document.all` as `superclass.prototype` case with DFG/FTL.
> +           Also, tweaks error message to reflect that `null` is allowed.

Can you describe what the change in behavior is? I assume it's that we say document.all can't be the superclass of a class expression?

> Source/JavaScriptCore/ChangeLog:14
> +        2. Renames is_object_or_null bytecode op to typeof_is_object, fixing confusing

Nit: fixing confusing => fixing the confusing.

> Source/JavaScriptCore/bytecode/BytecodeList.rb:336
> -        :is_object_or_null,
> +        :typeof_is_object,

Can you put this next to typeof_is_undefined.

> Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:191
> -    USES(OpIsObjectOrNull, operand)
> +    USES(OpTypeofIsObject, operand)

ditto on location.

> Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:498
> -    DEFS(OpIsObjectOrNull, dst)
> +    DEFS(OpTypeofIsObject, dst)

ditto on  location.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1463
> -    case IsObjectOrNull:
> +    case TypeOfIsObject:

Ditto on location

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1499
> -            case IsObjectOrNull:
> +            case TypeOfIsObject:

Ditto on location

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1695
> -        case IsObjectOrNull:
> +        case TypeOfIsObject:

Ditto on location I'm not going to comment on each of them for brevity but can you migrate all the various places.

[Alexey Shvayka]

Created attachment 406684 [details]
Patch

Relocate TypeOfIsObject next to TypeOfIsUndefined, clarify ChangeLog and fix typo.

[EWS]

Committed r265744: <https://trac.webkit.org/changeset/265744>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406684 [details].