Summary: | Refactor recompileAllJSFunctions() to be less expensive | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Hahnenberg <mhahnenberg> | ||||||||
Component: | JavaScriptCore | Assignee: | 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
Mark Hahnenberg
2012-03-05 14:46:40 PST
Created attachment 130212 [details]
Patch
Comment on attachment 130212 [details] Patch Attachment 130212 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11833012 Comment on attachment 130212 [details] Patch Attachment 130212 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11833017 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. (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. > I tried this and ran membuster on it and it used ~50% more memory on average.
Wow! OK, sold :).
More evidence for LLInt...
> 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.
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. Created attachment 130667 [details]
Patch
Comment on attachment 130667 [details]
Patch
r=me
Please apply this to Debugger.cpp too.
I believe you can remove the forEachCell function now.
(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 on attachment 130667 [details] Patch Clearing flags on attachment: 130667 Committed r110127: <http://trac.webkit.org/changeset/110127> All reviewed patches have been landed. Closing bug. Rolled out in bug 80562. Reopening. > + friend class DoublyLinkedListNode<FunctionExecutable>; I think this needs to be: + friend class WTF::DoublyLinkedListNode<FunctionExecutable>; Created attachment 134681 [details]
Patch
Comment on attachment 134681 [details] Patch Clearing flags on attachment: 134681 Committed r112624: <http://trac.webkit.org/changeset/112624> All reviewed patches have been landed. Closing bug. |