Bug 77761 - Split MarkedSpace into destructor and destructor-free subspaces
Summary: Split MarkedSpace into destructor and destructor-free subspaces
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on: 77600
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-03 11:49 PST by Mark Hahnenberg
Modified: 2012-02-12 10:30 PST (History)
7 users (show)

See Also:


Attachments
Patch (31.48 KB, patch)
2012-02-03 12:32 PST, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Bencher results (7.13 KB, text/plain)
2012-02-03 12:35 PST, Mark Hahnenberg
no flags Details
Trying to fix builds (34.47 KB, patch)
2012-02-03 13:01 PST, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (35.37 KB, patch)
2012-02-05 21:56 PST, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Fixing some build errors (35.60 KB, patch)
2012-02-06 08:11 PST, Mark Hahnenberg
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2012-02-03 11:49:26 PST
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.
Comment 1 Mark Hahnenberg 2012-02-03 11:51:56 PST
(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.
Comment 2 Mark Hahnenberg 2012-02-03 12:32:10 PST
Created attachment 125383 [details]
Patch
Comment 3 Mark Hahnenberg 2012-02-03 12:35:32 PST
Created attachment 125384 [details]
Bencher results

Slight loss < 1% on SunSpider, slight win on v8 and kraken (both < 1%). Overall, mostly neutral.
Comment 4 Mark Hahnenberg 2012-02-03 12:43:55 PST
(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.
Comment 5 Early Warning System Bot 2012-02-03 12:46:42 PST
Comment on attachment 125383 [details]
Patch

Attachment 125383 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11424326
Comment 6 Gyuyoung Kim 2012-02-03 12:55:54 PST
Comment on attachment 125383 [details]
Patch

Attachment 125383 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11424333
Comment 7 Mark Hahnenberg 2012-02-03 13:01:33 PST
Created attachment 125389 [details]
Trying to fix builds
Comment 8 Geoffrey Garen 2012-02-05 12:23:12 PST
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.
Comment 9 Mark Hahnenberg 2012-02-05 21:56:20 PST
Created attachment 125568 [details]
Patch
Comment 10 Gustavo Noronha (kov) 2012-02-05 22:15:22 PST
Comment on attachment 125568 [details]
Patch

Attachment 125568 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11429238
Comment 11 Early Warning System Bot 2012-02-05 22:21:45 PST
Comment on attachment 125568 [details]
Patch

Attachment 125568 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11431234
Comment 12 Gyuyoung Kim 2012-02-05 22:29:56 PST
Comment on attachment 125568 [details]
Patch

Attachment 125568 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11433158
Comment 13 Mark Hahnenberg 2012-02-06 08:11:13 PST
Created attachment 125649 [details]
Fixing some build errors
Comment 14 Geoffrey Garen 2012-02-06 11:52:43 PST
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.
Comment 15 Mark Hahnenberg 2012-02-06 11:55:27 PST
(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.
Comment 16 Geoffrey Garen 2012-02-06 16:02:56 PST
> 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.
Comment 17 Mark Hahnenberg 2012-02-07 11:28:00 PST
(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?
Comment 18 Mark Hahnenberg 2012-02-07 12:01:37 PST
(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?
Comment 19 Geoffrey Garen 2012-02-08 08:40:40 PST
> Maybe I could just move the sweep function out of the header and rename sweepSlowCase to something less related to speed?

Sounds good.
Comment 20 Geoffrey Garen 2012-02-08 08:42:29 PST
> 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.
Comment 21 Geoffrey Garen 2012-02-08 08:42:50 PST
Comment on attachment 125649 [details]
Fixing some build errors

r=me with the changes we've discussed, and a fixed Mac build.
Comment 22 Mark Hahnenberg 2012-02-10 15:14:51 PST
Committed r107445: <http://trac.webkit.org/changeset/107445>
Comment 23 Csaba Osztrogonác 2012-02-11 00:50:44 PST
(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?
Comment 24 Csaba Osztrogonác 2012-02-11 00:51:31 PST
I cc-ed Qt JSC experts too, could you check it?
Comment 25 Mark Hahnenberg 2012-02-11 17:28:21 PST
(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.
Comment 26 Csaba Osztrogonác 2012-02-12 10:30:33 PST
(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.