rdar://78175542
Created attachment 429011 [details] proposed patch.
Comment on attachment 429011 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=429011&action=review > Source/JavaScriptCore/bytecode/CodeBlock.cpp:2936 > + createRareDataIfNecessary(); > + auto& cachedIdentifierUids = m_rareData->m_cachedIdentifierUids; > + if (UNLIKELY(cachedIdentifierUids.size() != numberOfIdentifiers)) { > + cachedIdentifierUids.reserveInitialCapacity(numberOfIdentifiers); > + for (unsigned index = 0; index < unlinkedIdentifiers; ++index) { > + const Identifier& identifier = unlinkedCode->identifier(index); > + cachedIdentifierUids.add(identifier.impl()); > + } > + if (numberOfDFGIdentifiers) { > + ASSERT(JITCode::isOptimizingJIT(jitType())); > + auto& dfgIdentifiers = m_jitCode->dfgCommon()->m_dfgIdentifiers; > + for (unsigned index = 0; index < numberOfDFGIdentifiers; ++index) { > + const Identifier& identifier = dfgIdentifiers[index]; > + cachedIdentifierUids.add(identifier.impl()); > + } > + } > + } > + return cachedIdentifierUids.contains(uid); This function can be called from DFG compilation thread. So we need to have a lock.
Created attachment 429018 [details] proposed patch.
Comment on attachment 429018 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=429018&action=review > Source/JavaScriptCore/bytecode/CodeBlock.cpp:2919 > + if (!m_rareData || m_rareData->m_cachedIdentifierUids.size() != numberOfIdentifiers) { m_cachedIdentifierUids.size() is dangerous without taking a lock. If it is in the middle of growing, then it can be SEGV since HashSet stores its size into the backing memory. The other part looks good to me.
(In reply to Yusuke Suzuki from comment #4) > Comment on attachment 429018 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=429018&action=review > > > Source/JavaScriptCore/bytecode/CodeBlock.cpp:2919 > > + if (!m_rareData || m_rareData->m_cachedIdentifierUids.size() != numberOfIdentifiers) { > > m_cachedIdentifierUids.size() is dangerous without taking a lock. If it is > in the middle of growing, then it can be SEGV since HashSet stores its size > into the backing memory. > The other part looks good to me. I don't think the number of identifiers can grow by the time CacheableIdentifier::createFromIdentifierOwnedByCodeBlock() is called. The original code was iterating over existing identifiers and asserting that the specified identifier is found among the CodeBlock ones.
Created attachment 429025 [details] proposed patch.
Comment on attachment 429025 [details] proposed patch. Taking out of review first. Need to check one more thing.
Comment on attachment 429025 [details] proposed patch. OK, all is well. Just wanted to verify that m_jitCode->dfgCommon()->m_dfgIdentifiers is only set once, and not appended to. It is indeed only set once in DesiredIdentifiers::reallyAdd(). Hence, the number of identifiers are fixed and will not grow.
Comment on attachment 429025 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=429025&action=review > Source/JavaScriptCore/bytecode/CodeBlock.cpp:2919 > + if (m_cachedIdentifierUids.size() != numberOfIdentifiers) { There is no issue with growing this because the table is fixed size. Since we create it on the side and WTFMove into m_cachedIdentifierUids only after it is fully initialized, m_cachedIdentifierUids.size() will only ever see 0 or the real size.
Comment on attachment 429025 [details] proposed patch. Thanks for the review. Landing now.
Committed r277727 (237904@main): <https://commits.webkit.org/237904@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 429025 [details].
Also landed CLoop build fix in r277759: <http://trac.webkit.org/r277759>.