[JSC] StructureStubInfo's m_identifier should follow to the same protocol of inlineAccessBaseStructure
Created attachment 433831 [details] Patch
Comment on attachment 433831 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433831&action=review r=me > Source/JavaScriptCore/ChangeLog:10 > + m_identifier field too. This patch makes m_identifier follow to the same protocol of inlineAccessBaseStructure > + so that we fix this race issue too. I'd document just for completeness what we're doing specifically (so folks don't have to read r279813 change) > Source/JavaScriptCore/bytecode/StructureStubInfo.cpp:-422 > -void StructureStubInfo::setCacheType(const ConcurrentJSLockerBase&, CacheType newCacheType) I would keep this function. It's nice to document/enforce that we're holding the lock. See https://trac.webkit.org/changeset/264388/webkit
Comment on attachment 433831 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433831&action=review >> Source/JavaScriptCore/bytecode/StructureStubInfo.cpp:-422 >> -void StructureStubInfo::setCacheType(const ConcurrentJSLockerBase&, CacheType newCacheType) > > I would keep this function. It's nice to document/enforce that we're holding the lock. See https://trac.webkit.org/changeset/264388/webkit Maybe make it inline since it's no-op function now?
Comment on attachment 433831 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433831&action=review >>> Source/JavaScriptCore/bytecode/StructureStubInfo.cpp:-422 >>> -void StructureStubInfo::setCacheType(const ConcurrentJSLockerBase&, CacheType newCacheType) >> >> I would keep this function. It's nice to document/enforce that we're holding the lock. See https://trac.webkit.org/changeset/264388/webkit > > Maybe make it inline since it's no-op function now? For now, I'll just make it ALWAYS_INLINE void StructureStubInfo::setCacheType(const ConcurrentJSLockerBase&, CacheType newCacheType) { m_cacheType = newCacheType; } But personally, thread-safety-annotation would be better to me in this case since we already get Locker parameter in each function which modifies this field. (but currently, not sure how to annotate it via that if it depends on CodeBlock's lock.
Committed r280066 (239797@main): <https://commits.webkit.org/239797@main>
<rdar://problem/80813147>