[webkit-reviews] review granted: [Bug 209266] [JSC] StructureStubInfo::bufferedStructures should not ref/deref UniquedStringImpl : [Attachment 393939] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 209266: [JSC] StructureStubInfo::bufferedStructures should not ref/deref
UniquedStringImpl
https://bugs.webkit.org/show_bug.cgi?id=209266

Attachment 393939: Patch

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




--- Comment #5 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 393939
  --> https://bugs.webkit.org/attachment.cgi?id=393939
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?