Bug 61134 – Make Executables release their JIT code as soon as they become dead

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Make Executable's release their JIT code as soon as they become dead

[Oliver Hunt]

Created attachment 94099 [details]
Patch

[Geoffrey Garen]

Comment on attachment 94099 [details]
Patch

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

r- because I think you're missing an implementation of executableFinalizer().

> Source/JavaScriptCore/runtime/Executable.cpp:58
> +WeakHandleOwner* ExecutableBase::executableFinalizer()
> +{
> +    return 0;
> +}

This seems wrong. Who actually provides the executable finalizer?

> Source/JavaScriptCore/runtime/Executable.h:65
> +#if ENABLE(JIT)
> +            HandleSlot slot = globalData.heap.allocateGlobalHandle();
> +            HandleHeap* handleHeap = HandleHeap::heapFor(slot);
> +            handleHeap->makeWeak(slot, executableFinalizer());
> +            handleHeap->writeBarrier(slot, this);
> +            *slot = this;

It's a shame that clients have to duplicate all this error-prone code. The Weak<T> class was an attempt to reduce error-prone duplication. I think it could be adapted to this purpose if you added leak() and adopt() features to Weak<T>.

[Oliver Hunt]

(In reply to comment #2)
> (From update of attachment 94099 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94099&action=review
> 
> r- because I think you're missing an implementation of executableFinalizer().
> 
> > Source/JavaScriptCore/runtime/Executable.cpp:58
> > +WeakHandleOwner* ExecutableBase::executableFinalizer()
> > +{
> > +    return 0;
> > +}
> 
> This seems wrong. Who actually provides the executable finalizer?

The patch was meant to, i suspect i posted an incomplete patch but i'm redoing perf numbers to be sure.

> 
> > Source/JavaScriptCore/runtime/Executable.h:65
> > +#if ENABLE(JIT)
> > +            HandleSlot slot = globalData.heap.allocateGlobalHandle();
> > +            HandleHeap* handleHeap = HandleHeap::heapFor(slot);
> > +            handleHeap->makeWeak(slot, executableFinalizer());
> > +            handleHeap->writeBarrier(slot, this);
> > +            *slot = this;
> 
> It's a shame that clients have to duplicate all this error-prone code. The Weak<T> class was an attempt to reduce error-prone duplication. I think it could be adapted to this purpose if you added leak() and adopt() features to Weak<T>.

agreed.

[Oliver Hunt]

Created attachment 94104 [details]
Patch

[Geoffrey Garen]

Comment on attachment 94104 [details]
Patch

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

r=me

> Source/JavaScriptCore/heap/Weak.h:133
> +        setSlot(0);

Might as well assert that you have a finalizer here. It's not a guarantee that you won't leak, but not having a finalizer is a guarantee that you will leak.

> Source/JavaScriptCore/runtime/Executable.cpp:56
> +    static ExecutableFinalizer* finalizer = new ExecutableFinalizer;

Please use DEFINE_STATIC_LOCAL, in accord with our style.

[Early Warning System Bot]

Comment on attachment 94104 [details]
Patch

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

[Oliver Hunt]

Committed r86883: <http://trac.webkit.org/changeset/86883>