CodeBlockSet should be generational

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Hahnenberg]

This is a big overhead during EdenCollections. We should think about how we can generationalize our CodeBlock lifetimes.

[Mark Hahnenberg]

Created attachment 222707 [details]
Patch

[Geoffrey Garen]

Comment on attachment 222707 [details]
Patch

r=me

[Mark Hahnenberg]

(In reply to comment #1)
> Created an attachment (id=222707) [details]
> Patch

Hmm, I think this patch has issues. The correctness of the weakReferenceHarvester stuff in which CodeBlocks participate is dependent on visiting all marked CodeBlocks during a collection. Otherwise we won't propagate liveness for all the things that hang off of CodeBlock that need it.

Another alternative is to just ignore all new CodeBlocks (i.e. auto-promote, pretenure, etc.). We'd still have to visit them to LUB value profiles.

[Mark Hahnenberg]

Created attachment 228214 [details]
Patch

[Mark Hahnenberg]

Created attachment 228216 [details]
Patch

[Geoffrey Garen]

Comment on attachment 228216 [details]
Patch

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

r=me

> Source/JavaScriptCore/bytecode/CodeBlock.h:1004
> +        // During EdenCollections all CodeBlocks that are visited are assumed to be live.

Would be nice to explain our reasoning here: It's probably cheaper to assume liveness, and we don't want to worry about write barrier subtleties required by the mutually weak liveness methodology.

> Source/JavaScriptCore/heap/CodeBlockSet.cpp:71
> +void CodeBlockSet::clearAllMarks()
>  {
> -    for (CodeBlock* codeBlock : m_set) {
> +    promoteYoungCodeBlocks();
> +    for (CodeBlock* codeBlock : m_oldCodeBlocks) {
>          codeBlock->m_mayBeExecuting = false;
>          codeBlock->m_visitAggregateHasBeenCalled = false;
>      }
>  }

It feels strange for "clearAllMarks" to promote. And might this get us into a bad state if deleteAllCompiledCode is called at the start of an eden collection? Similarly strange, though less so, for deleteUnmarkedAndUnreferenced to promote.

Can we make the caller promote instead? For example, Heap::deleteAllCompiledCode could run only during full collections, and Heap::markRoots could eagerly promote code blocks before full collections and lazily promote code blocks after eden collections. Or, Heap::resetAllocators() could promote code blocks unconditionally, and Heap::deleteAllCompiledCode could run after Heap::resetAllocators().

[Geoffrey Garen]

Comment on attachment 228216 [details]
Patch

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

> Source/JavaScriptCore/heap/CodeBlockSet.cpp:135
> +        m_oldCodeBlocks.remove(codeBlock);

Needs early return.

[Mark Hahnenberg]

Created attachment 228428 [details]
Final patch

[Mark Hahnenberg]

Comment on attachment 228428 [details]
Final patch

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

> Source/JavaScriptCore/bytecode/CodeBlock.h:151
> +    template <typename Functor> void forEachRelatedCodeBlock(Functor&& functor)
> +    {
> +        Functor f(std::forward<Functor>(functor));
> +        Vector<CodeBlock*, 4> codeBlocks;
> +        codeBlocks.append(this);
> +
> +        while (!codeBlocks.isEmpty()) {
> +            CodeBlock* currentCodeBlock = codeBlocks.takeLast();
> +            f(currentCodeBlock);
> +
> +            if (CodeBlock* alternative = currentCodeBlock->alternative())
> +                codeBlocks.append(alternative);
> +            if (CodeBlock* osrEntryBlock = currentCodeBlock->specialOSREntryBlockOrNull())
> +                codeBlocks.append(osrEntryBlock);
> +        }
> +    }

I had to add handling of the OSR entry block along with the alternative blocks. To make it more general, I turned it into a DFS of a DAG. There's no defense against cycles.

[Geoffrey Garen]

Comment on attachment 228428 [details]
Final patch

r=me

[WebKit Commit Bot]

Comment on attachment 228428 [details]
Final patch

Clearing flags on attachment: 228428

Committed r166678: <http://trac.webkit.org/changeset/166678>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.