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.
Created attachment 350587 [details] Patch
Comment on attachment 350587 [details] Patch r=me
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.
Created attachment 350666 [details] Patch
Comment on attachment 350666 [details] Patch r=me if EWS bots are happy.
Comment on attachment 350666 [details] Patch Clearing flags on attachment: 350666 Committed r236430: <https://trac.webkit.org/changeset/236430>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44741589>