[webkit-reviews] review denied: [Bug 80330] Remove recompileAllJSFunctions() from Heap::collectAllGarbage() : [Attachment 130212] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 80330: Remove recompileAllJSFunctions() from Heap::collectAllGarbage()
https://bugs.webkit.org/show_bug.cgi?id=80330

Attachment 130212: Patch
https://bugs.webkit.org/attachment.cgi?id=130212&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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(sp
ecializationKind());
> +	       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.