Bug 226072 – [JSC] Use DataIC for AccessCase

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] Use DataIC for AccessCase

[Yusuke Suzuki]

Created attachment 429275 [details]
Patch

[Yusuke Suzuki]

Created attachment 429325 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 429325 [details]
Patch

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

> PerformanceTests/Speedometer/resources/benchmark-runner.js:137
> +        $vm.dataLog(
> +            $vm.preciseTime() + ": " + context + "; numberOfOSRExits = " + $vm.numberOfOSRExits() +

The way we'd add WebKit/JSC specific logic to use a plan file run-benchmark uses.
We should probably be applying a patch that modifies _writeMark instead.

[Yusuke Suzuki]

Created attachment 429833 [details]
Patch

[Yusuke Suzuki]

Created attachment 429955 [details]
Patch

[Yusuke Suzuki]

(In reply to Ryosuke Niwa from comment #3)
> Comment on attachment 429325 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429325&action=review
> 
> > PerformanceTests/Speedometer/resources/benchmark-runner.js:137
> > +        $vm.dataLog(
> > +            $vm.preciseTime() + ": " + context + "; numberOfOSRExits = " + $vm.numberOfOSRExits() +
> 
> The way we'd add WebKit/JSC specific logic to use a plan file run-benchmark
> uses.
> We should probably be applying a patch that modifies _writeMark instead.

This is just for debugging and will not appear in the final (r?) patch :)

[Yusuke Suzuki]

Created attachment 429959 [details]
Patch

[Yusuke Suzuki]

Created attachment 429960 [details]
Patch

[Yusuke Suzuki]

Created attachment 429961 [details]
Patch

[Yusuke Suzuki]

Created attachment 429963 [details]
Patch

[Yusuke Suzuki]

Created attachment 429973 [details]
Patch

[Yusuke Suzuki]

Created attachment 429975 [details]
Patch

[Yusuke Suzuki]

Created attachment 429992 [details]
Patch

[Yusuke Suzuki]

Created attachment 429994 [details]
Patch

[Yusuke Suzuki]

Created attachment 429995 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/78610881>

[Yusuke Suzuki]

1. I should check the structure access while generating code to make the depende t explicit to AccessCase since Structure can be destroyed when CodeBlock is not destroyed yet. (Maybe, Transition case would be affected. And I should ise StructureID in AccessCase instead.)
2. I cleaned up InstanceOf generator and caused the crash now, need to check what’s wrong.

[Yusuke Suzuki]

Ah, one thing I need to do is resolving the problem with GC-managed cells in AccessCases. It is possible that,

1. Shared JIT Stub holds AccessCases.
2. Shared JIT Stub is not destroyed, but cells in AccessCases are already daed. This can happen because the stub will be destroyed only after CodeBlock is destroyed.
3. We accidentally grab the cells that has the same pointer (while they are different structure IDs), and then we incorrectly use the almost-dying stub since this is still registered.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #18)
> Ah, one thing I need to do is resolving the problem with GC-managed cells in
> AccessCases. It is possible that,
> 
> 1. Shared JIT Stub holds AccessCases.
> 2. Shared JIT Stub is not destroyed, but cells in AccessCases are already
> daed. This can happen because the stub will be destroyed only after
> CodeBlock is destroyed.
> 3. We accidentally grab the cells that has the same pointer (while they are
> different structure IDs), and then we incorrectly use the almost-dying stub
> since this is still registered.

Make weakReferences to embeddedStructureIDs. And put it in the stub so that we use these embedded IDs as the comparison target.
We should also make AccessCase's structure StructureID since the actually embedded one is StructureID.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #19)
> (In reply to Yusuke Suzuki from comment #18)
> > Ah, one thing I need to do is resolving the problem with GC-managed cells in
> > AccessCases. It is possible that,
> > 
> > 1. Shared JIT Stub holds AccessCases.
> > 2. Shared JIT Stub is not destroyed, but cells in AccessCases are already
> > daed. This can happen because the stub will be destroyed only after
> > CodeBlock is destroyed.
> > 3. We accidentally grab the cells that has the same pointer (while they are
> > different structure IDs), and then we incorrectly use the almost-dying stub
> > since this is still registered.
> 
> Make weakReferences to embeddedStructureIDs. And put it in the stub so that
> we use these embedded IDs as the comparison target.
> We should also make AccessCase's structure StructureID since the actually
> embedded one is StructureID.

Will update the table by marking live ones at CodeBlock::finalizeUnconditionally.

[Yusuke Suzuki]

Created attachment 430184 [details]
Patch

[Yusuke Suzuki]

Created attachment 430206 [details]
Patch

[Yusuke Suzuki]

Created attachment 430207 [details]
Patch

[Yusuke Suzuki]

Need to look into instanceof repatching

[Yusuke Suzuki]

Created attachment 430230 [details]
Patch

[Yusuke Suzuki]

Created attachment 430234 [details]
Patch

[Yusuke Suzuki]

Created attachment 430406 [details]
Patch

[Yusuke Suzuki]

Created attachment 430426 [details]
Patch

[Saam Barati]

Comment on attachment 430426 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:22
> +

Maybe it's worth calling out some of the calling convention stuff you did?

> Source/JavaScriptCore/assembler/LinkBuffer.h:238
> +        // ASSERT(call.isFlagSet(Call::Linkable));
> +        // ASSERT(!call.isFlagSet(Call::Near));

oops

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1598
> +        if (codeBlock->useSharedIC())

nit: I think this should be called "useDataICs" since we need to always do this for data ICs, regardless of if this particular PolymorphicAccess gets shared or not

> Source/JavaScriptCore/bytecode/AccessCase.cpp:2238
> +bool AccessCase::equalInSharing(const AccessCase& lhs, const AccessCase& rhs)

nit: Maybe just "canBeShared" as a name?

> Source/JavaScriptCore/bytecode/AccessCase.cpp:2256
> +    if (lhs.m_identifier != rhs.m_identifier)
> +        return false;

if the offsets and structure's are equal, can't this be an assert?

> Source/JavaScriptCore/bytecode/AccessCase.h:282
> +    AccessCase(AccessCase&& other)
> +        : m_type(WTFMove(other.m_type))
> +        , m_state(WTFMove(other.m_state))
> +        , m_viaProxy(WTFMove(other.m_viaProxy))
> +        , m_offset(WTFMove(other.m_offset))
> +        , m_structure(WTFMove(other.m_structure))
> +        , m_conditionSet(WTFMove(other.m_conditionSet))
> +        , m_polyProtoAccessChain(WTFMove(other.m_polyProtoAccessChain))
> +        , m_identifier(WTFMove(other.m_identifier))
> +    { }

why does the default version no longer work?

> Source/JavaScriptCore/bytecode/AccessCase.h:387
> +                return a.m_wrapped != b.m_wrapped;

why is this safe compared to ==?

> Source/JavaScriptCore/bytecode/AccessCase.h:456
> +        const FixedVector<RefPtr<AccessCase>>& m_keys;

nit: Why not call this m_cases?

> Source/JavaScriptCore/bytecode/AccessCase.h:474
> +        m_stubs.add(key);

WTFMove(key)?

> Source/JavaScriptCore/bytecode/InlineAccess.cpp:384
> +void InlineAccess::rewireStubAsJumpInAccessNotUsingInlineCode(CodeBlock* codeBlock, StructureStubInfo& stubInfo, CodeLocationLabel<JITStubRoutinePtrTag> target)

I don't quite get the significance of this name. We do sometimes replace the inline code.

> Source/JavaScriptCore/bytecode/InlineAccess.cpp:401
> +    CCallHelpers jit;
> +
> +    auto jump = jit.jump();
> +
> +    // We don't need a nop sled here because nobody should be jumping into the middle of an IC.
> +    bool needsBranchCompaction = false;
> +    LinkBuffer linkBuffer(jit, stubInfo.start, jit.m_assembler.buffer().codeSize(), LinkBuffer::Profile::InlineCache, JITCompilationMustSucceed, needsBranchCompaction);
> +    RELEASE_ASSERT(linkBuffer.isValid());
> +    linkBuffer.link(jump, target);
> +
> +    FINALIZE_CODE(linkBuffer, NoPtrTag, "InlineAccess: linking constant jump");

Can this just call resetStubAsJumpInAccess?

> Source/JavaScriptCore/bytecode/InlineAccess.cpp:417
> +                jit.move(CCallHelpers::TrustedImmPtr(&stubInfo), stubInfo.m_stubInfoGPR);
> +                jit.call(CCallHelpers::Address(stubInfo.m_stubInfoGPR, StructureStubInfo::offsetOfCodePtr()), JITStubRoutinePtrTag);
> +                auto jump = jit.jump();

we're guaranteed we have room to spit out all this code?

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:364
> +        for (StructureID weakReference : m_stubRoutine->weakReferences()) {

nit: Since this is no longer generic, can we name "weakReferences()" to "weakStructures"?

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:538
> +    // Getter / Setter relies on stack-pointer adjustment, which is tied to the linked CodeBlock, which makes this code unshareable.

nit: I wouldn't state this comment as an impossibility. We could share it, but we'd just need to read stack offset from the CodeBlock at runtime via the code we emit.

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:564
> +    if (needsSymbolPropertyCheck || needsStringPropertyCheck || needsInt32PropertyCheck)

I sorta feel like it should just be if stubInfo.hasConstantIdentifier is false, then canBeShared is false?

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:589
> +            // We have no guarantee that stack-pointer is the expected one. This is not a problem if we are not having JS getter / setter calls since stack-pointer is

nit: "if we are not having JS getter / setter calls" => "if we do not have JS getter / setter calls"

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:590
> +            // callee-save register and it is kept beyond calls. However, our JS executable call does not save stack-pointer. So we are adjusting stack-pointer after

nit: "callee-save register and it is kept beyond calls" => "a callee-save register in the C calling convention"

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:593
> +            jit.addPtr(CCallHelpers::TrustedImm32(codeBlock->stackPointerOffset() * sizeof(Register)), GPRInfo::callFrameRegister, CCallHelpers::stackPointerRegister);

I actually don't get this. We are emitting the same thing after getter/setter call

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:812
> +    if (codeBlock->useSharedIC() && canBeShared) {
> +        SharedJITStubSet::Searcher searcher {
> +            stubInfo.baseGPR,
> +            stubInfo.valueGPR,
> +            stubInfo.regs.thisGPR,
> +            stubInfo.m_stubInfoGPR,
> +            stubInfo.usedRegisters,
> +            keys,
> +            weakReferences,
> +        };
> +        stub = vm.m_sharedJITStubs->find(searcher);
> +        if (stub) {
> +            dataLogLnIf(PolymorphicAccessInternal::verbose, "Found existing code stub ", stub->code());
> +            return finishCodeGeneration(WTFMove(stub));
> +        }
> +    }

Why do we do this after generating so much JIT code? Why not do it before generating any JIT code?

> Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:193
> +
> +

nit: undo

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:959
> +        appendCallSetResult(CCallHelpers::Address(GPRInfo::nonArgGPR0, address.offset), result);

why is nonArgGPR0 the right register?

> Source/JavaScriptCore/jit/JITCode.h:167
> +        return Options::useSharedICInOptimizingJIT();

have some variant of our tests turn this on so it doesn't bit rot?

> Source/JavaScriptCore/jit/JITStubRoutine.h:42
> +#if USE(JSVALUE64)
> +using StructureID = uint32_t;
> +#else
> +using StructureID = Structure*;
> +#endif

Why do we need to repeat this?

> Source/JavaScriptCore/runtime/VM.cpp:663
> +#if ENABLE(JIT)
> +    m_sharedJITStubs = nullptr;
> +#endif

Why is this explicitly needed?

[Saam Barati]

Comment on attachment 430426 [details]
Patch

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

> Source/JavaScriptCore/bytecode/AccessCase.h:430
> +                    const auto& avec = a.m_wrapped->keys();
> +                    const auto& bvec = b.m_keys;

name nit:
aCases and bCases?

> Source/JavaScriptCore/bytecode/AccessCase.h:436
> +                    for (unsigned index = 0; index < bvec.size(); ++index) {
> +                        if (!AccessCase::equalInSharing(*avec[index], *bvec[index]))
> +                            return false;
> +                    }

This is sensitive to the order. I wonder if we can make it agnostic to the order in which the cases happen. Maybe file a FIXME for that?

> Source/JavaScriptCore/bytecode/AccessCase.h:438
> +                    const auto& aweak = a.m_wrapped->weakReferences();
> +                    const auto& bweak = b.m_weakReferences;

name nit: aWeak bWeak

> Source/JavaScriptCore/bytecode/AccessCase.h:444
> +                    for (unsigned i = 0, size = aweak.size(); i < size; ++i) {
> +                        if (aweak[i] != bweak[i])
> +                            return false;
> +                    }

this seems sensitive to order. Should we make it so we don't care about the order?

> Source/JavaScriptCore/bytecode/AccessCase.h:466
> +        static bool equal(const Hash::Key key, AccessCaseJITStubRoutine* stub)

const Hash::Key& instead of const Hash::Key?

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:788
> +        // In ARM64, we do not push anything on stack specially.

maybe also add an ASSERT(isARM64()) to document that this is a requirement right now, so that if/when we do things on x86, we know we have to fix this code.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:959
>> +        appendCallSetResult(CCallHelpers::Address(GPRInfo::nonArgGPR0, address.offset), result);
> 
> why is nonArgGPR0 the right register?

I see, so setupArgumentsForIndirectCall puts the address base into nonArgGPR0?

[Yusuke Suzuki]

Comment on attachment 430426 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:22
>> +
> 
> Maybe it's worth calling out some of the calling convention stuff you did?

Sounds good.

>> Source/JavaScriptCore/assembler/LinkBuffer.h:238
>> +        // ASSERT(!call.isFlagSet(Call::Near));
> 
> oops

Oops, this comment-out is not longer necessary. Removed.

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:1598
>> +        if (codeBlock->useSharedIC())
> 
> nit: I think this should be called "useDataICs" since we need to always do this for data ICs, regardless of if this particular PolymorphicAccess gets shared or not

Changed.

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:2238
>> +bool AccessCase::equalInSharing(const AccessCase& lhs, const AccessCase& rhs)
> 
> nit: Maybe just "canBeShared" as a name?

Changed.

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:2256
>> +        return false;
> 
> if the offsets and structure's are equal, can't this be an assert?

Changed.

>> Source/JavaScriptCore/bytecode/AccessCase.h:282
>> +    { }
> 
> why does the default version no longer work?

This is because AccessCase now inherits ThreadSafeRefCounted. And ThreadSafeRefCounted's copy/move-constructor is `delete`-ed. This makes `= default` compile error.

>> Source/JavaScriptCore/bytecode/AccessCase.h:387
>> +                return a.m_wrapped != b.m_wrapped;
> 
> why is this safe compared to ==?

Oops, this is old code. Deleted.

>> Source/JavaScriptCore/bytecode/AccessCase.h:430
>> +                    const auto& bvec = b.m_keys;
> 
> name nit:
> aCases and bCases?

Changed.

>> Source/JavaScriptCore/bytecode/AccessCase.h:436
>> +                    }
> 
> This is sensitive to the order. I wonder if we can make it agnostic to the order in which the cases happen. Maybe file a FIXME for that?

Yes, but I think in many cases, this would be the same. I'll put FIXME.

>> Source/JavaScriptCore/bytecode/AccessCase.h:438
>> +                    const auto& bweak = b.m_weakReferences;
> 
> name nit: aWeak bWeak

Changed.

>> Source/JavaScriptCore/bytecode/AccessCase.h:444
>> +                    }
> 
> this seems sensitive to order. Should we make it so we don't care about the order?

I think, in reality, this order does not matter. We put StructureIDs here, and they are populated from object-property-condition-set. And that set is ordered by prototype-chain ordering.
So I think there is no case that A and B AccessCases have different ordering weak references while they are the same.
Of course, if we make AccessCase attempt case-insensitive, this can be different, but so far, I think FIXME is enough.

>> Source/JavaScriptCore/bytecode/AccessCase.h:456
>> +        const FixedVector<RefPtr<AccessCase>>& m_keys;
> 
> nit: Why not call this m_cases?

Originally, it was only keys, but now weakReferences are added. Changed it to m_cases.

>> Source/JavaScriptCore/bytecode/AccessCase.h:466
>> +        static bool equal(const Hash::Key key, AccessCaseJITStubRoutine* stub)
> 
> const Hash::Key& instead of const Hash::Key?

Changed.

>> Source/JavaScriptCore/bytecode/AccessCase.h:474
>> +        m_stubs.add(key);
> 
> WTFMove(key)?

Changed.

>> Source/JavaScriptCore/bytecode/InlineAccess.cpp:384
>> +void InlineAccess::rewireStubAsJumpInAccessNotUsingInlineCode(CodeBlock* codeBlock, StructureStubInfo& stubInfo, CodeLocationLabel<JITStubRoutinePtrTag> target)
> 
> I don't quite get the significance of this name. We do sometimes replace the inline code.

Changed it to InlineAccess.

>> Source/JavaScriptCore/bytecode/InlineAccess.cpp:401
>> +    FINALIZE_CODE(linkBuffer, NoPtrTag, "InlineAccess: linking constant jump");
> 
> Can this just call resetStubAsJumpInAccess?

Not possible since resetStubAsJumpInAccess is resetting a jump to slow-path while it is setting a jump to the target.

>> Source/JavaScriptCore/bytecode/InlineAccess.cpp:417
>> +                auto jump = jit.jump();
> 
> we're guaranteed we have room to spit out all this code?

Yes

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:364
>> +        for (StructureID weakReference : m_stubRoutine->weakReferences()) {
> 
> nit: Since this is no longer generic, can we name "weakReferences()" to "weakStructures"?

Sounds good. Changed.

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:538
>> +    // Getter / Setter relies on stack-pointer adjustment, which is tied to the linked CodeBlock, which makes this code unshareable.
> 
> nit: I wouldn't state this comment as an impossibility. We could share it, but we'd just need to read stack offset from the CodeBlock at runtime via the code we emit.

Yes, but I think the current code is not doing that, and this comment is stating why we can't share it.
I think this comment is worth keeping. So, when we would like to change them to be shared, we can read this comment, and we can know what is preventing from sharing.

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:564
>> +    if (needsSymbolPropertyCheck || needsStringPropertyCheck || needsInt32PropertyCheck)
> 
> I sorta feel like it should just be if stubInfo.hasConstantIdentifier is false, then canBeShared is false?

Yeah, changed.

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:589
>> +            // We have no guarantee that stack-pointer is the expected one. This is not a problem if we are not having JS getter / setter calls since stack-pointer is
> 
> nit: "if we are not having JS getter / setter calls" => "if we do not have JS getter / setter calls"

Changed.

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:590
>> +            // callee-save register and it is kept beyond calls. However, our JS executable call does not save stack-pointer. So we are adjusting stack-pointer after
> 
> nit: "callee-save register and it is kept beyond calls" => "a callee-save register in the C calling convention"

Changed.

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:593
>> +            jit.addPtr(CCallHelpers::TrustedImm32(codeBlock->stackPointerOffset() * sizeof(Register)), GPRInfo::callFrameRegister, CCallHelpers::stackPointerRegister);
> 
> I actually don't get this. We are emitting the same thing after getter/setter call

This is super important. Without that, JetStream2 crashes almost immediately.

The reason is that, unfortunately, our JS calling convention / OSR exit / tail-call etc. does not ensure that stack-pointer is `CCallHelpers::TrustedImm32(codeBlock->stackPointerOffset() * sizeof(Register)), GPRInfo::callFrameRegister`. When using this IC in baseline JIT, this is possible that SP is not `CCallHelpers::TrustedImm32(codeBlock->stackPointerOffset() * sizeof(Register)), GPRInfo::callFrameRegister`.
But it is OK: SP is always larger-than-or-equal-to `CCallHelpers::TrustedImm32(codeBlock->stackPointerOffset() * sizeof(Register)), GPRInfo::callFrameRegister`, and it should work.

However, in this IC case, that does not work well. We tag the return-address with SP. But after calling getter / setter, SP will be adjusted to `CCallHelpers::TrustedImm32(codeBlock->stackPointerOffset() * sizeof(Register)), GPRInfo::callFrameRegister`, which can be different from the original stack-pointer!
Then, when returning, we see the different stack-pointer, and crash due to PAC failure.

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:788
>> +        // In ARM64, we do not push anything on stack specially.
> 
> maybe also add an ASSERT(isARM64()) to document that this is a requirement right now, so that if/when we do things on x86, we know we have to fix this code.

Sounds good. Changed.

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:812
>> +    }
> 
> Why do we do this after generating so much JIT code? Why not do it before generating any JIT code?

Because that made code significantly complicated (basically, almost double the same code). We need to collect

1. watchpoints based on the current state of ObjectPropertyConditionSet (and in some cases we collect more)
2. weak-structure IDs collected through emission

If we don't do that, we need to maintain almost the same code which only collects structure-ids and watchpoints. And this can easily introduce bugs if only one of them is changed.

>> Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:193
>> +
> 
> nit: undo

Fixed.

>>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:959
>>> +        appendCallSetResult(CCallHelpers::Address(GPRInfo::nonArgGPR0, address.offset), result);
>> 
>> why is nonArgGPR0 the right register?
> 
> I see, so setupArgumentsForIndirectCall puts the address base into nonArgGPR0?

Yes. nonArgGPR0 is ensured that it is not used for any arguments and it is caller-save register. So, at the point of calling a function, scratching it is OK.

>> Source/JavaScriptCore/jit/JITCode.h:167
>> +        return Options::useSharedICInOptimizingJIT();
> 
> have some variant of our tests turn this on so it doesn't bit rot?

Sounds good.

>> Source/JavaScriptCore/jit/JITStubRoutine.h:42
>> +#endif
> 
> Why do we need to repeat this?

Because I saw compile-error if we include `Structure.h` here. We need forward declaration of them.

>> Source/JavaScriptCore/runtime/VM.cpp:663
>> +#endif
> 
> Why is this explicitly needed?

By killing it explicitly, we can ensure that `m_sharedJITStubs` is killed by checking `m_vm.m_sharedJITStubs`. And when shutting down VM, it is possible that,

1. m_vm.m_sharedJITStubs is killed
2. Then, some of JITStubRoutine is destroyed and it was tied to m_vm.m_sharedJITStubs.

So, in the destructor of (2), we can check `if (m_vm.m_sharedJITStubs)` to ensure that we can avoid accessing it if it is already destroyed.

[Saam Barati]

Comment on attachment 430426 [details]
Patch

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

> Source/JavaScriptCore/bytecode/InlineAccess.cpp:413
> +                CCallHelpers jit;

One more suggestion that I just remembered:

I think the JIT code in this patch that overwrites existing code in memory would be a bit cleaner if it uses the new helper I added:
CCallHelpers::emitJITCodeOver API. It handles a lot of the abstraction for you.

[Filip Pizlo]

Comment on attachment 430426 [details]
Patch

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

Wow, what a patch.  R=me with mine and Saam's comments.

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:276
> +        auto myCase = WTFMove(originalCasesToAdd[i]);

I don't like the use of auto right here.  I think this code will get read a lot, and is subtle, so having this thing's type would be great here.

> Source/JavaScriptCore/jit/GCAwareJITStubRoutine.h:93
> +class AccessCaseJITStubRoutine : public GCAwareJITStubRoutine {

Is this used to represent a single access case, or a PolymorphicAccess-compiled polymorphic thing with many cases?

If the latter, I suggest picking a different name.  Maybe PolymorphicAccessJITStubRoutine.

[Yusuke Suzuki]

Comment on attachment 430426 [details]
Patch

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

>>> Source/JavaScriptCore/bytecode/AccessCase.cpp:2256
>>> +        return false;
>> 
>> if the offsets and structure's are equal, can't this be an assert?
> 
> Changed.

Ah, this was not true. If it is `Miss` IC, then offset is always -1 and the map can be the same while identifier is different.

>> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:276
>> +        auto myCase = WTFMove(originalCasesToAdd[i]);
> 
> I don't like the use of auto right here.  I think this code will get read a lot, and is subtle, so having this thing's type would be great here.

Changed it to RefPtr<AccessCase>.

>> Source/JavaScriptCore/jit/GCAwareJITStubRoutine.h:93
>> +class AccessCaseJITStubRoutine : public GCAwareJITStubRoutine {
> 
> Is this used to represent a single access case, or a PolymorphicAccess-compiled polymorphic thing with many cases?
> 
> If the latter, I suggest picking a different name.  Maybe PolymorphicAccessJITStubRoutine.

Yeah, this is code for PolymorphicAccess compiled one. Changed it to PolymorphicAccessJITStubRoutine.

[Yusuke Suzuki]

Created attachment 430887 [details]
Patch

[Yusuke Suzuki]

Created attachment 430888 [details]
Patch

[Yusuke Suzuki]

Created attachment 430889 [details]
Patch

[Yusuke Suzuki]

Created attachment 430934 [details]
Patch

[Yusuke Suzuki]

Waiting for 6 hours and still not getting EWS run :(

[Yusuke Suzuki]

OK looks fine. Failing ones are flaky ones.

[Yusuke Suzuki]

Committed r278656 (238638@main): <https://commits.webkit.org/238638@main>

[Fujii Hironori]

WinCairo clang-cl builds got broken since this change.
  Bug 226850 – clang-cl: JIT.h(966,67): error: no viable conversion from 'JSC::AbstractMacroAssembler<JSC::X86Assembler>::Address' to 'FunctionPtr<CFunctionPtrTag>'