Bug 225948 – Speed up debug validation in CacheableIdentifier::createFromIdentifierOwnedByCodeBlock().

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

[Mark Lam]

rdar://78175542

[Mark Lam]

Created attachment 429011 [details]
proposed patch.

[Yusuke Suzuki]

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.

[Mark Lam]

Created attachment 429018 [details]
proposed patch.

[Yusuke Suzuki]

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.

[Mark Lam]

(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.

[Mark Lam]

Created attachment 429025 [details]
proposed patch.

[Mark Lam]

Comment on attachment 429025 [details]
proposed patch.

Taking out of review first.  Need to check one more thing.

[Mark Lam]

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.

[Mark Lam]

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.

[Mark Lam]

Comment on attachment 429025 [details]
proposed patch.

Thanks for the review.  Landing now.

[EWS]

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].

[Mark Lam]

Also landed CLoop build fix in r277759: <http://trac.webkit.org/r277759>.