Bug 80330 – Refactor recompileAllJSFunctions() to be less expensive

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Hahnenberg]

recompileAllJSFunctions() is a very expensive function, as it is iterates every single object in the heap. We call it rather frequently, especially when navigating web pages. We can do this much more intelligently by doing essentially the same thing during mark/sweep collection. In doing so, we avoid having to touch every object in the heap, which should benefit page load performance. The standard benchmarks we track should be unaffected.

[Mark Hahnenberg]

Created attachment 130212 [details]
Patch

[Mark Hahnenberg]

<rdar://problem/10898464>

[Build Bot]

Comment on attachment 130212 [details]
Patch

Attachment 130212 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11833012

[Early Warning System Bot]

Comment on attachment 130212 [details]
Patch

Attachment 130212 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11833017

[Geoffrey Garen]

Comment on attachment 130212 [details]
Patch

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

It just occurred to me that the solution can be much simpler: Why not just remove the call to recompileAllJSFunctions() from collectAllGarbage()? We only added it to work around inline caches keeping things alive too long, and I think Phil fixed that bug through the weak reference harvester mechanism.

I think you should try that and test the SunSpider result.

Anyway, what follows is a review of your patch as is.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1773
> +    if (m_globalData->heap.collectionType() == Heap::BerserkerCollection && !m_globalData->dynamicGlobalObject) {
> +        if (m_ownerExecutable->classInfo() == &FunctionExecutable::s_info) {
> +            // Since we got into this unconditional finalizer list, we know our alternative got into the list too,
> +            // so it will get blown away as well. We cast to void here to avoid the unused return value warning.
> +            // If we didn't release our alternative here, we'd destroy it and then re-encounter it later in the list, 
> +            // which would be very bad because it would no longer have a valid vtable pointer.
> +            if (!!m_alternative)
> +                (void)m_alternative.leakPtr();
> +            static_cast<FunctionExecutable*>(m_ownerExecutable.get())->discardCodeOfKind(specializationKind());
> +            return;
> +        }
> +    }

It would be clearer and less error-prone if the Executable were "in charge" of finalization -- it's odd for a child object to manage the lifetime of its parent, and you've had to contort the code a bit to accomplish that.

> Source/JavaScriptCore/heap/Heap.h:142
> +        enum CollectionType { NoCollection, NormalCollection, BerserkerCollection };
> +        CollectionType collectionType() { return m_collectionType; }

You can just use the SweepToggle enum. This enum is redundant with it.

> Source/JavaScriptCore/heap/Heap.h:242
> +        CollectionType m_collectionType;

I think this should be a data member of the SlotVisitor, since it only has meaning during the visit cycle.

[Mark Hahnenberg]

(In reply to comment #5)
> (From update of attachment 130212 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130212&action=review
> 
> It just occurred to me that the solution can be much simpler: Why not just remove the call to recompileAllJSFunctions() from collectAllGarbage()? We only added it to work around inline caches keeping things alive too long, and I think Phil fixed that bug through the weak reference harvester mechanism.
> 
> I think you should try that and test the SunSpider result.

I tried this and ran membuster on it and it used ~50% more memory on average.

[Geoffrey Garen]

> I tried this and ran membuster on it and it used ~50% more memory on average.

Wow! OK, sold :).

More evidence for LLInt...

[Mark Hahnenberg]

> It would be clearer and less error-prone if the Executable were "in charge" of finalization -- it's odd for a child object to manage the lifetime of its parent, and you've had to contort the code a bit to accomplish that.

The CodeBlock effectively takes charge of its own lifetime. It merely gets its parent to do the dirty work of actually deleting itself. I suppose this could be considered a violation of the semantics of the FunctionExecutable's OwnPtr? Anyways, the primary reason my code is contorted is because finalizeUnconditionally() is a virtual function, which we can't add to FunctionExecutable itself. I could think of three ways to get around this:

1) Have a proxy object that we add to the list of unconditional finalizers that would just call discardCode() on the FunctionExecutable. This would require additional (malloc) allocations during the marking phase. 
2) Use CodeBlock's existing finalizeUnconditionally(). This has been shown to require a tad of smelly hackery.
3) Use (or create) some other mechanism than UnconditionalFinalizer to blow away CodeBlocks on the main thread.

[Mark Hahnenberg]

Renaming bug to more accurately reflect what we're going to do. Instead of leveraging UnconditionalFinalizer for discarding CodeBlocks, we'll just keep a separate linked list of FunctionExecutables and iterate over them to discard their compiled code.

[Mark Hahnenberg]

Created attachment 130667 [details]
Patch

[Geoffrey Garen]

Comment on attachment 130667 [details]
Patch

r=me

Please apply this to Debugger.cpp too.

I believe you can remove the forEachCell function now.

[Mark Hahnenberg]

(In reply to comment #11)
> (From update of attachment 130667 [details])
> r=me
> 
> Please apply this to Debugger.cpp too.
> 
> I believe you can remove the forEachCell function now.

I don't think we can do that in Debugger just with keeping track of FunctionExecutables. That particular Recompiler actually searches the Heap for JSFunctions, which it then uses to get the scope chain along with the executable. There's no way to get back from FunctionExecutable to JSFunction so that we would be able to  duplicate that functionality.

[WebKit Review Bot]

Comment on attachment 130667 [details]
Patch

Clearing flags on attachment: 130667

Committed r110127: <http://trac.webkit.org/changeset/110127>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

Rolled out in bug 80562. Reopening.

> +        friend class DoublyLinkedListNode<FunctionExecutable>;

I think this needs to be:

+        friend class WTF::DoublyLinkedListNode<FunctionExecutable>;

[Mark Hahnenberg]

Created attachment 134681 [details]
Patch

[WebKit Review Bot]

Comment on attachment 134681 [details]
Patch

Clearing flags on attachment: 134681

Committed r112624: <http://trac.webkit.org/changeset/112624>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.