1697504 - Prefer JSObject subclasses in CacheIR code to avoid emitting unreachable code

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P1)

Description

[André Bargull [:anba]]

TestMatchingReceiver has if-statements to handle ProxyObject and TypedObject types, even though this function is only called with NativeObject. This can easily be fixed if we properly pass NativeObject* instead of JSObject* through all callers. As part of this bug, I'll also remove unnecessary rooting, for example in IRGenerator::emitIdGuard, which contains a rooted and a non-rooted parameter, which is slightly confusing.

Comment 1

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 1: Use ProxyObject* instead of JSObject* in various CacheIR methods. r=jandem!

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 2: Return a NativeObject* for the no-gc variant of NativeLookupPropertyInline. r=jandem!

Use NativeObject* for the no-gc variant of NativeLookupPropertyInline and
then update all callers to use NativeObject*, too.

Depends on D107838

Comment 3

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 3: Use NativeObject* in EmitCallGetterResultGuards. r=jandem!

Use NativeObject* for EmitCallGetterResultGuards, this allows us to directly
call TestMatchingNativeReceiver.

Depends on D107839

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 4: Use NativeObject* in HasPropIRGenerator::tryAttachSlotDoesNotExist(). r=jandem!

Similar to part 3, this allows to directly call TestMatchingNativeReceiver().

Depends on D107841

Comment 5

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 5: Use NativeObject* in SetPropIRGenerator::tryAttachSetter(). r=jandem!

Similar to part 3, this allows to directly call TestMatchingNativeReceiver().

Depends on D107842

Comment 6

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 6: Use NativeObject* in EmitReadSlotGuard. r=jandem!

Similar to part 3, this allows to directly call TestMatchingNativeReceiver().

TestMatchingReceiver() is no longer used, so we can remove it now, too.

Depends on D107843

Comment 7

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 7: Remove GuardTypeDescr CacheIR op. r=jandem!

GuardTypeDescr is no longer used after part 6.

Also removes the corresponding MacroAssembler methods.

Depends on D107844

Comment 8

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 8: Fix includes to "wasm/TypedObject.h". r=jandem!

Part 7 showed that there are incorrect includes to "wasm/TypedObject.h".
(MacroAssembler used TypedObject without including it.)

Depends on D107845

Comment 9

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 9: Use NativeObject* in GeneratePrototypeHoleGuards, GuardReceiverProto, and ShapeGuardProtoChain. r=jandem!

Use NativeObject* in GeneratePrototypeHoleGuards, GuardReceiverProto, and
ShapeGuardProtoChain, so it's more obvious why calling JSObject::staticPrototype()
is correct.

Depends on D107846

Comment 10

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 10: Use NativeObject* in TestMatchingHolder. r=jandem!

Pass through NativeObject* to TestMatchingHolder() instead of asserting
the argument JSObject is a native object.

Depends on D107847

Comment 11

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 11: Use NativeObject* for IsCacheableGetProp{ReadSlot,Call}(). r=jandem!

IsCacheableGetPropReadSlot() and IsCacheableGetPropCall() are only applicable
for native objects, therefore use NativeObject* for both functions.

Depends on D107848

Comment 12

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 12: Use NativeObject* for IsCacheableSetPropCall{Native,Scripted}(). r=jandem!

Similar to part 11, use NativeObject* for these two functions.

Depends on D107849

Comment 13

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 13: Use NativeObject* for IsCacheableProtoChain. r=jandem!

IsCacheableProtoChain() is only called with NativeObject*. Also turn all
IsCacheableProtoChain() checks into assertions to match the current code.

The comment about lookupProperty potentially modifying the prototype chain
dates back to bug 729099. At that point of time, the code was calling
JSObject::lookupProperty() (today's js::LookupProperty()), which could
indeed modify the prototype chain. But today CacheIR is always calling
js::LookupPropertyPure() (or similar functions), which can't alter any
prototypes.

Depends on D107850

Comment 14

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 14: Use NativeObject* for remaining functions. r=jandem!

Using NativeObject* doesn't give any extra value except for documenting which
types of objects we currently expect here.

Depends on D107851

Comment 15

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 15: Use raw pointers in CanAttachNativeGetProp. r=jandem!

No functions called in CanAttachNativeGetProp() can GC, so we can safely use
raw pointers.

Depends on D107852

Comment 16

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 16: Use raw pointers in CanAttachDOMCall and CanAttachDOMGetterSetter. r=jandem!

The DOMInstanceClassHasProtoAtDepth call in CanAttachDOMCall() can't be
analysed by the hazard analysis because it happens through a pointer to a
function. This means any GC values must either be rooted or alternatively
JS::AutoSuppressGCAnalysis could be used to disable the hazard analysis.

Consistently use handles for both functions so callers can't use raw pointers
without causing a compiler error.

Depends on D107854

Comment 17

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 17: Avoid more unnecessary rooting in CacheIR. r=jandem!

Replace more rooted variables with raw pointers. Also avoids in some places
to have rooted and raw pointers next to each other.

Depends on D107855

Comment 18

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 18: Add missing 'static' modifier. r=jandem!

Depends on D107856

Comment 19

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 19: Remove unnecessary rooting from CanAttachGlobalName. r=jandem!

Depends on D107857

Comment 20

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 20: Remove unnecessary rooting from CanAttachSetter. r=jandem!

Depends on D107860

Comment 21

[André Bargull [:anba]]

Attachment: Bug 1697504 - Part 21: Remove unnecessary rooting from CanAttachNativeSetSlot. r=jandem!

Depends on D107861

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6091f772179c
Part 1: Use ProxyObject* instead of JSObject* in various CacheIR methods. r=jandem
https://hg.mozilla.org/integration/autoland/rev/1a1d48764556
Part 2: Return a NativeObject* for the no-gc variant of NativeLookupPropertyInline. r=jandem
https://hg.mozilla.org/integration/autoland/rev/655671c2ec69
Part 3: Use NativeObject* in EmitCallGetterResultGuards. r=jandem
https://hg.mozilla.org/integration/autoland/rev/3c84769b0a00
Part 4: Use NativeObject* in HasPropIRGenerator::tryAttachSlotDoesNotExist(). r=jandem
https://hg.mozilla.org/integration/autoland/rev/0daa0d35c49c
Part 5: Use NativeObject* in SetPropIRGenerator::tryAttachSetter(). r=jandem
https://hg.mozilla.org/integration/autoland/rev/1ea7ec3d8f5c
Part 6: Use NativeObject* in EmitReadSlotGuard. r=jandem
https://hg.mozilla.org/integration/autoland/rev/766614ff771c
Part 7: Remove GuardTypeDescr CacheIR op. r=jandem
https://hg.mozilla.org/integration/autoland/rev/3cdb584bd4a0
Part 8: Fix includes to "wasm/TypedObject.h". r=jandem
https://hg.mozilla.org/integration/autoland/rev/94be6f952008
Part 9: Use NativeObject* in GeneratePrototypeHoleGuards, GuardReceiverProto, and ShapeGuardProtoChain. r=jandem
https://hg.mozilla.org/integration/autoland/rev/711b00030864
Part 10: Use NativeObject* in TestMatchingHolder. r=jandem
https://hg.mozilla.org/integration/autoland/rev/ccf84078cea2
Part 11: Use NativeObject* for IsCacheableGetProp{ReadSlot,Call}(). r=jandem
https://hg.mozilla.org/integration/autoland/rev/04321bf57b5b
Part 12: Use NativeObject* for IsCacheableSetPropCall{Native,Scripted}(). r=jandem
https://hg.mozilla.org/integration/autoland/rev/3a2ced62cd9f
Part 13: Use NativeObject* for IsCacheableProtoChain. r=jandem
https://hg.mozilla.org/integration/autoland/rev/ed113f45c482
Part 14: Use NativeObject* for remaining functions. r=jandem
https://hg.mozilla.org/integration/autoland/rev/861100e3f84b
Part 15: Use raw pointers in CanAttachNativeGetProp. r=jandem
https://hg.mozilla.org/integration/autoland/rev/6ed359874293
Part 16: Use raw pointers in CanAttachDOMCall and CanAttachDOMGetterSetter. r=jandem
https://hg.mozilla.org/integration/autoland/rev/a1e3663e5bc0
Part 17: Avoid more unnecessary rooting in CacheIR. r=jandem
https://hg.mozilla.org/integration/autoland/rev/8d7bba4146f5
Part 18: Add missing 'static' modifier. r=jandem
https://hg.mozilla.org/integration/autoland/rev/b551678dc0de
Part 19: Remove unnecessary rooting from CanAttachGlobalName. r=jandem
https://hg.mozilla.org/integration/autoland/rev/98642a9e548e
Part 20: Remove unnecessary rooting from CanAttachSetter. r=jandem
https://hg.mozilla.org/integration/autoland/rev/8249396ef069
Part 21: Remove unnecessary rooting from CanAttachNativeSetSlot. r=jandem

Comment 23

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/6091f772179c
https://hg.mozilla.org/mozilla-central/rev/1a1d48764556
https://hg.mozilla.org/mozilla-central/rev/655671c2ec69
https://hg.mozilla.org/mozilla-central/rev/3c84769b0a00
https://hg.mozilla.org/mozilla-central/rev/0daa0d35c49c
https://hg.mozilla.org/mozilla-central/rev/1ea7ec3d8f5c
https://hg.mozilla.org/mozilla-central/rev/766614ff771c
https://hg.mozilla.org/mozilla-central/rev/3cdb584bd4a0
https://hg.mozilla.org/mozilla-central/rev/94be6f952008
https://hg.mozilla.org/mozilla-central/rev/711b00030864
https://hg.mozilla.org/mozilla-central/rev/ccf84078cea2
https://hg.mozilla.org/mozilla-central/rev/04321bf57b5b
https://hg.mozilla.org/mozilla-central/rev/3a2ced62cd9f
https://hg.mozilla.org/mozilla-central/rev/ed113f45c482
https://hg.mozilla.org/mozilla-central/rev/861100e3f84b
https://hg.mozilla.org/mozilla-central/rev/6ed359874293
https://hg.mozilla.org/mozilla-central/rev/a1e3663e5bc0
https://hg.mozilla.org/mozilla-central/rev/8d7bba4146f5
https://hg.mozilla.org/mozilla-central/rev/b551678dc0de
https://hg.mozilla.org/mozilla-central/rev/98642a9e548e
https://hg.mozilla.org/mozilla-central/rev/8249396ef069