Bug 77761 – Split MarkedSpace into destructor and destructor-free subspaces

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

Now that MarkedAllocators are responsible for allocating their own MarkedBlocks, we can use them to divide up the MarkedSpace into subspaces that split objects up according to whether they require a destructor call when sweeping the MarkedBlocks. We can start off by allocating JSFinalObjects and JSArrays in this new destructor-free subspace. As we move more objects off of requiring destructors (anything that has an ASSERT_HAS_TRIVIAL_DESTRUCTOR), we can move more objects into this subspace. The long-term vision is to have all objects allocated out of this destructor-free space.

[Mark Hahnenberg]

(In reply to comment #0)
> (anything that has an ASSERT_HAS_TRIVIAL_DESTRUCTOR)

And anything that also has a trivial destroy() method, where trivial means the same thing that it does for normal destructors.

[Mark Hahnenberg]

Created attachment 125383 [details]
Patch

[Mark Hahnenberg]

Created attachment 125384 [details]
Bencher results

Slight loss < 1% on SunSpider, slight win on v8 and kraken (both < 1%). Overall, mostly neutral.

[Mark Hahnenberg]

(In reply to comment #2)
> Created an attachment (id=125383) [details]
> Patch

It appears that every compiler except clang hates the way that I've specialized these templates. I'll work on appeasing them.

[Early Warning System Bot]

Comment on attachment 125383 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 125383 [details]
Patch

Attachment 125383 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11424333

[Mark Hahnenberg]

Created attachment 125389 [details]
Trying to fix builds

[Geoffrey Garen]

Comment on attachment 125389 [details]
Trying to fix builds

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

Patch looks correct. Some architecture comments below. Also, I'd like to see results from those two micro benchmarks I mentioned by email.

> Source/JavaScriptCore/heap/MarkedSpace.h:95
> +    struct Subspace {
> +        FixedArray<MarkedAllocator, preciseCount> preciseSizeClasses;
> +        FixedArray<MarkedAllocator, impreciseCount> impreciseSizeClasses;
> +    };

Minor nit: You should rename all "size class" variables to "allocator" to match your class renaming.

> Source/JavaScriptCore/runtime/JSObject.h:399
> +template<> inline void* allocateCell<JSFinalObject>(Heap& heap)

Let's not duplicate this function for each destructor-free class. Instead, you can specialize allocateCell<T> based on class traits, much like hash traits:

template<class T, NeedsDestructorArg = NeedsDestructor<T>> allocateCell(...) 
{
    if (NeedsDestructorArg::value) { ... } else { ... }
}

// Write manual specializations for this struct template if you care about non-clang compilers.
template<class T> struct NeedsDestructor {
    static bool value = !has_non_trivial_destructor(T);
};

This would also eliminate the need for class friendship and the DestructorFreeAllocator helper. You can make both allocateWithDestructor and allocateWithoutDestructor private, and make allocateCell<T> a friend.

[Mark Hahnenberg]

Created attachment 125568 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 125568 [details]
Patch

Attachment 125568 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11429238

[Early Warning System Bot]

Comment on attachment 125568 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 125568 [details]
Patch

Attachment 125568 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11433158

[Mark Hahnenberg]

Created attachment 125649 [details]
Fixing some build errors

[Geoffrey Garen]

Comment on attachment 125649 [details]
Fixing some build errors

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

> Source/JavaScriptCore/heap/MarkedBlock.h:214
> +    inline MarkedBlock::FreeCell* MarkedBlock::sweep(SweepMode sweepMode)

I don't think distinguishing sweep from sweepSlowCase adds value. Eager sweep isn't a case we need to specialize for with inlining, and it would be better not to have multiple copies of the sweep function template. I would merge sweep with sweepSlowCase in an out-of-line function.

> Source/JavaScriptCore/runtime/JSCell.h:319
> +    struct CellAllocator {

Let's not add the extra indirection of a class here. You can just declare allocateCell a friend of the heap directly, and put this code inside allocateCell.

> Source/WebCore/bindings/js/JSDOMWindowShell.cpp:171
> +    return heap.allocateWithDestructor(size);

Why does JSDOMWindowShell need its own operator new? It would be much better not to have this, and not to have a forward declaration of a WebCore data type in JavaScriptCore's Heap.h.

[Mark Hahnenberg]

(In reply to comment #14)
> > Source/JavaScriptCore/runtime/JSCell.h:319
> > +    struct CellAllocator {
> 
> Let's not add the extra indirection of a class here. You can just declare allocateCell a friend of the heap directly, and put this code inside allocateCell.

The extra indirection is because older compilers don't allow you to have default template parameters on functions, only classes/structs.

[Geoffrey Garen]

> The extra indirection is because older compilers don't allow you to have default template parameters on functions, only classes/structs.

Interesting.

OK. Let's not make it a parameter at all -- no one uses the parameter anyway. Instead, you can just use NeedsDestructor<T>::value directly inside the function.

[Mark Hahnenberg]

(In reply to comment #14)
> (From update of attachment 125649 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125649&action=review
> 
> > Source/JavaScriptCore/heap/MarkedBlock.h:214
> > +    inline MarkedBlock::FreeCell* MarkedBlock::sweep(SweepMode sweepMode)
> 
> I don't think distinguishing sweep from sweepSlowCase adds value. Eager sweep isn't a case we need to specialize for with inlining, and it would be better not to have multiple copies of the sweep function template. I would merge sweep with sweepSlowCase in an out-of-line function.

The main reason I split them up is because it makes the code in sweepSlowCase a little cleaner. I don't have to check the cellsNeedDestruction flag in each of the branches of the switch statement to pass the correct template parameter to specializedSweep. Instead, I just do a single check in sweep, and then choose which version of sweepSlowCase I want. Maybe I could just move the sweep function out of the header and rename sweepSlowCase to something less related to speed?

[Mark Hahnenberg]

(In reply to comment #16)
> > The extra indirection is because older compilers don't allow you to have default template parameters on functions, only classes/structs.
> 
> Interesting.
> 
> OK. Let's not make it a parameter at all -- no one uses the parameter anyway. Instead, you can just use NeedsDestructor<T>::value directly inside the function.

Another thing that might be an issue is destroy() functions that do other things than call the destructor. Maybe we should declare that it is invalid to do so?

[Geoffrey Garen]

> Maybe I could just move the sweep function out of the header and rename sweepSlowCase to something less related to speed?

Sounds good.

[Geoffrey Garen]

> Another thing that might be an issue is destroy() functions that do other things than call the destructor. Maybe we should declare that it is invalid to do so?

This is only really an issue if a class has a meaningful destroy() function but no C++ destructor -- the destroy() function won't be called, even though it needs to be.

I think you can guard against this by ASSERTing that T::destroy == JSCell::destroy, or something like that.

[Geoffrey Garen]

Comment on attachment 125649 [details]
Fixing some build errors

r=me with the changes we've discussed, and a fixed Mac build.

[Mark Hahnenberg]

Committed r107445: <http://trac.webkit.org/changeset/107445>

[Csaba Osztrogonác]

(In reply to comment #22)
> Committed r107445: <http://trac.webkit.org/changeset/107445>

Reopen, because it caused an assertion for all tests on Qt in debug mode:
ASSERTION FAILED: cell->classInfo() != &JSFinalObject::s_info
../../../../Source/JavaScriptCore/heap/MarkedBlock.cpp(74) : void JSC::MarkedBlock::callDestructor(JSC::JSCell*)

Could you fix it, please?

[Csaba Osztrogonác]

I cc-ed Qt JSC experts too, could you check it?

[Mark Hahnenberg]

(In reply to comment #23)
> (In reply to comment #22)
> > Committed r107445: <http://trac.webkit.org/changeset/107445>
> 
> Reopen, because it caused an assertion for all tests on Qt in debug mode:
> ASSERTION FAILED: cell->classInfo() != &JSFinalObject::s_info
> ../../../../Source/JavaScriptCore/heap/MarkedBlock.cpp(74) : void JSC::MarkedBlock::callDestructor(JSC::JSCell*)
> 
> Could you fix it, please?

Potential fix in http://trac.webkit.org/changeset/107495. Please verify.

[Csaba Osztrogonác]

(In reply to comment #25)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > Committed r107445: <http://trac.webkit.org/changeset/107445>
> > 
> > Reopen, because it caused an assertion for all tests on Qt in debug mode:
> > ASSERTION FAILED: cell->classInfo() != &JSFinalObject::s_info
> > ../../../../Source/JavaScriptCore/heap/MarkedBlock.cpp(74) : void JSC::MarkedBlock::callDestructor(JSC::JSCell*)
> > 
> > Could you fix it, please?
> 
> Potential fix in http://trac.webkit.org/changeset/107495. Please verify.

It fixed the assertion, thanks for the quick fix.