Bug 189897 – Add forEach method for iterating CodeBlock's ValueProfiles

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

[Tadeu Zagallo]

Add method to abstract how we find ValueProfiles in a CodeBlock in preparation for https://bugs.webkit.org/show_bug.cgi?id=189785, when ValueProfiles will be stored in the MetadataTable.

[Tadeu Zagallo]

Created attachment 350587 [details]
Patch

[Saam Barati]

Comment on attachment 350587 [details]
Patch

r=me

[Mark Lam]

Comment on attachment 350587 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350587&action=review

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2577
> +        totalNumberOfValueProfiles++;

I don't think you should compute the total this way.  The totalNumberOfValueProfiles(), numberOfArgumentValueProfiles(), and numberOfValueProfiles() are all static and fixed once the CodeBlock is created, right?  When adding the Metadata later, just increment a m_numberOfMetadataValueProfiles count if the Metadata contains a ValueProfile.  Make totalNumberOfValueProfiles() include m_numberOfMetadataValueProfiles.  You also need a new numberOfNonArgumentValueProfiles() which returns the sum of m_numberOfMetadataValueProfiles and numberOfValueProfiles().  Some of the existing code that is using numberOfValueProfiles() actually need to be using numberOfNonArgumentValueProfiles() instead.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2653
>      if ((!numberOfValueProfiles() || (double)numberOfLiveNonArgumentValueProfiles / numberOfValueProfiles() >= Options::desiredProfileLivenessRate())
> -        && (!totalNumberOfValueProfiles() || (double)numberOfSamplesInProfiles / ValueProfile::numberOfBuckets / totalNumberOfValueProfiles() >= Options::desiredProfileFullnessRate())
> +        && (!totalNumberOfValueProfiles || (double)numberOfSamplesInProfiles / ValueProfile::numberOfBuckets / totalNumberOfValueProfiles >= Options::desiredProfileFullnessRate())
>          && static_cast<unsigned>(m_optimizationDelayCounter) + 1 >= Options::minimumOptimizationDelay())
>          return true;

This is where we were using the numberOfValueProfiles() but what we really want is numberOfNonArgumentValueProfiles().  Previously, these 2 values are the same.  That will no longer be the case once you introduce Metadata ValueProfiles.  If you don't reflect this change, then the heuristics will change and performance characteristics may also change.  I think we should keep the heuristics static while changing the bytecodes, and not inadvertently changing them this way.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:-2699
> -    dataLog("ValueProfile for ", *this, ":\n");

We still need this line.

> Source/JavaScriptCore/bytecode/CodeBlock.h:433
> +        auto getFromAllValueProfiles = [&](unsigned index) -> ValueProfile& {
> +            if (index < numberOfArgumentValueProfiles())
> +                return valueProfileForArgument(index);
> +            return valueProfile(index - numberOfArgumentValueProfiles());
> +        };
> +
> +        unsigned totalNumberOfValueProfiles = numberOfArgumentValueProfiles() + numberOfValueProfiles();
> +
> +        for (unsigned i = 0; i < totalNumberOfValueProfiles; ++i) {
> +            ValueProfile& profile = getFromAllValueProfiles(i);
> +            func(profile);
> +        }

I think getFromAllValueProfiles here is superfluous.  Since we control the iteration here, can't you just express this as:

    for (unsigned i = 0; i < numberOfArgumentValueProfiles(); ++i)
        func(valueProfileForArgument(i))
    for (unsigned i = 0; i < numberOfValueProfiles(); ++i)
        func(valueProfile(i))

Expressing it this way also makes it easier to add a 3rd loop later for Metadata ValueProfiles.

> Source/JavaScriptCore/bytecode/CodeBlock.h:881
> +    unsigned numberOfArgumentValueProfiles()
> +    {
> +        ASSERT(m_numParameters >= 0);
> +        ASSERT(m_argumentValueProfiles.size() == static_cast<unsigned>(m_numParameters) || !vm()->canUseJIT());
> +        return m_argumentValueProfiles.size();
> +    }
> +
> +    ValueProfile& valueProfileForArgument(unsigned argumentIndex)
> +    {
> +        ASSERT(vm()->canUseJIT()); // This is only called from the various JIT compilers or places that first check numberOfArgumentValueProfiles before calling this.
> +        ValueProfile& result = m_argumentValueProfiles[argumentIndex];
> +        ASSERT(result.m_bytecodeOffset == -1);
> +        return result;
> +    }
> +

Why move this here from the above location?  The above location is grouped with other ValueProfile methods and seem like a better place to keep these methods.  In both cases, the methods are public.  So, this move looks unnecessary to me.

[Tadeu Zagallo]

Created attachment 350666 [details]
Patch

[Mark Lam]

Comment on attachment 350666 [details]
Patch

r=me if EWS bots are happy.

[WebKit Commit Bot]

Comment on attachment 350666 [details]
Patch

Clearing flags on attachment: 350666

Committed r236430: <https://trac.webkit.org/changeset/236430>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44741589>