Bug 209266 – [JSC] StructureStubInfo::bufferedStructures should not ref/deref UniquedStringImpl

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

[Yusuke Suzuki]

This data structure can be destroyed in CodeBlock::finalizeUnconditionally. So it should not include Strings.

1. Can we just set AtomStringTable when executing CodeBlock::finalizeUnconditionally?

This does not work correctly. Our Web Worker implementation is releasing heapAccess() when finishing code execution and waiting for runloop message.
This means that CodeBlock::finalizeUnconditionally can potentially work concurrently to Web Worker's main thread.

[Yusuke Suzuki]

<rdar://problem/60508312>

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #0)
> 1. Can we just set AtomStringTable when executing
> CodeBlock::finalizeUnconditionally?
> 
> This does not work correctly. Our Web Worker implementation is releasing
> heapAccess() when finishing code execution and waiting for runloop message.
> This means that CodeBlock::finalizeUnconditionally can potentially work
> concurrently to Web Worker's main thread.

2. Can we fix this issue by giving up resetJITData call in CodeBlock::finalizeUnconditionally?

No since we are calling visitWeakReferences(), which removes some of buffered-structures in CodeBlock::finalizeUnconditionally anyway.

[Yusuke Suzuki]

Created attachment 393939 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 393939 [details]
Patch

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

> Source/JavaScriptCore/jit/JITOperations.cpp:-2239
> -            if (stubInfo->considerCaching(vm, codeBlock, baseValue.structureOrNull()))

This patch also fixes DeleteByVal's considerCaching. It was not getting CacheableIdentifier.

[Saam Barati]

Comment on attachment 393939 [details]
Patch

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

r=me

Nice find

> Source/JavaScriptCore/ChangeLog:25
> +        concurrent collector to run this, we introduce m_bufferedStructuresLock in StructureStubInfo to guard m_bufferedStructures.

aren't we always holding the code block's lock in both cases?

> Source/JavaScriptCore/bytecode/StructureStubInfo.h:102
> +    ALWAYS_INLINE bool considerCaching(VM& vm, CodeBlock* codeBlock, Structure* structure, CacheableIdentifier impl = CacheableIdentifier())

I wonder if making this have a default argument was a mistake

> Source/JavaScriptCore/bytecode/StructureStubInfo.h:291
> +        using KeyTraits = SimpleClassHashTraits<BufferedStructure>;

nit: Maybe static assert that emptyValueIsZero for documentation purpose?

[Yusuke Suzuki]

Comment on attachment 393939 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:25
>> +        concurrent collector to run this, we introduce m_bufferedStructuresLock in StructureStubInfo to guard m_bufferedStructures.
> 
> aren't we always holding the code block's lock in both cases?

We are not holding a lock while executing considerCaching. I think fine-grained locking here is nice.

>> Source/JavaScriptCore/bytecode/StructureStubInfo.h:102
>> +    ALWAYS_INLINE bool considerCaching(VM& vm, CodeBlock* codeBlock, Structure* structure, CacheableIdentifier impl = CacheableIdentifier())
> 
> I wonder if making this have a default argument was a mistake

I think,

1. Adding `considerCachingById` and `considerCachingByVal`
2. `considerCachingById` internally calls `considerCaching` with CacheableIdentifier()

would be nice. Fixed.

>> Source/JavaScriptCore/bytecode/StructureStubInfo.h:291
>> +        using KeyTraits = SimpleClassHashTraits<BufferedStructure>;
> 
> nit: Maybe static assert that emptyValueIsZero for documentation purpose?

Sounds nice. Added.

[Yusuke Suzuki]

Committed r258732: <https://trac.webkit.org/changeset/258732>