Bug 189897 - Add forEach method for iterating CodeBlock's ValueProfiles
Summary: Add forEach method for iterating CodeBlock's ValueProfiles
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-23 13:04 PDT by Tadeu Zagallo
Modified: 2018-09-24 15:04 PDT (History)
7 users (show)

See Also:


Attachments
Patch (11.70 KB, patch)
2018-09-23 13:05 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (9.42 KB, patch)
2018-09-24 12:30 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tadeu Zagallo 2018-09-23 13:04:14 PDT
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.
Comment 1 Tadeu Zagallo 2018-09-23 13:05:19 PDT
Created attachment 350587 [details]
Patch
Comment 2 Saam Barati 2018-09-24 10:46:07 PDT
Comment on attachment 350587 [details]
Patch

r=me
Comment 3 Mark Lam 2018-09-24 11:19:39 PDT
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.
Comment 4 Tadeu Zagallo 2018-09-24 12:30:25 PDT
Created attachment 350666 [details]
Patch
Comment 5 Mark Lam 2018-09-24 12:37:33 PDT
Comment on attachment 350666 [details]
Patch

r=me if EWS bots are happy.
Comment 6 WebKit Commit Bot 2018-09-24 15:03:44 PDT
Comment on attachment 350666 [details]
Patch

Clearing flags on attachment: 350666

Committed r236430: <https://trac.webkit.org/changeset/236430>
Comment 7 WebKit Commit Bot 2018-09-24 15:03:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2018-09-24 15:04:19 PDT
<rdar://problem/44741589>