Bug 80330

Summary: Refactor recompileAllJSFunctions() to be less expensive
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 80562    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Mark Hahnenberg 2012-03-05 14:46:40 PST
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.
Comment 1 Mark Hahnenberg 2012-03-05 15:21:53 PST
Created attachment 130212 [details]
Patch
Comment 2 Mark Hahnenberg 2012-03-05 15:23:55 PST
<rdar://problem/10898464>
Comment 3 Build Bot 2012-03-05 15:34:39 PST
Comment on attachment 130212 [details]
Patch

Attachment 130212 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11833012
Comment 4 Early Warning System Bot 2012-03-05 15:53:04 PST
Comment on attachment 130212 [details]
Patch

Attachment 130212 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11833017
Comment 5 Geoffrey Garen 2012-03-05 17:34:18 PST
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.
Comment 6 Mark Hahnenberg 2012-03-05 17:35:57 PST
(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.
Comment 7 Geoffrey Garen 2012-03-05 17:58:14 PST
> I tried this and ran membuster on it and it used ~50% more memory on average.

Wow! OK, sold :).

More evidence for LLInt...
Comment 8 Mark Hahnenberg 2012-03-05 18:31:57 PST
> 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.
Comment 9 Mark Hahnenberg 2012-03-07 11:56:32 PST
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.
Comment 10 Mark Hahnenberg 2012-03-07 12:21:46 PST
Created attachment 130667 [details]
Patch
Comment 11 Geoffrey Garen 2012-03-07 12:27:18 PST
Comment on attachment 130667 [details]
Patch

r=me

Please apply this to Debugger.cpp too.

I believe you can remove the forEachCell function now.
Comment 12 Mark Hahnenberg 2012-03-07 13:28:03 PST
(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.
Comment 13 WebKit Review Bot 2012-03-07 18:14:54 PST
Comment on attachment 130667 [details]
Patch

Clearing flags on attachment: 130667

Committed r110127: <http://trac.webkit.org/changeset/110127>
Comment 14 WebKit Review Bot 2012-03-07 18:14:59 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Geoffrey Garen 2012-03-07 20:30:36 PST
Rolled out in bug 80562. Reopening.

> +        friend class DoublyLinkedListNode<FunctionExecutable>;

I think this needs to be:

+        friend class WTF::DoublyLinkedListNode<FunctionExecutable>;
Comment 16 Mark Hahnenberg 2012-03-29 15:25:34 PDT
Created attachment 134681 [details]
Patch
Comment 17 WebKit Review Bot 2012-03-29 17:36:55 PDT
Comment on attachment 134681 [details]
Patch

Clearing flags on attachment: 134681

Committed r112624: <http://trac.webkit.org/changeset/112624>
Comment 18 WebKit Review Bot 2012-03-29 17:37:00 PDT
All reviewed patches have been landed.  Closing bug.