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.
<rdar://problem/60508312>
(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.
Created attachment 393939 [details] Patch
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.
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?
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.
Committed r258732: <https://trac.webkit.org/changeset/258732>