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.
(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.
Created attachment 125383 [details] Patch
Created attachment 125384 [details] Bencher results Slight loss < 1% on SunSpider, slight win on v8 and kraken (both < 1%). Overall, mostly neutral.
(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 on attachment 125383 [details] Patch Attachment 125383 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11424326
Comment on attachment 125383 [details] Patch Attachment 125383 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11424333
Created attachment 125389 [details] Trying to fix builds
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.
Created attachment 125568 [details] Patch
Comment on attachment 125568 [details] Patch Attachment 125568 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11429238
Comment on attachment 125568 [details] Patch Attachment 125568 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11431234
Comment on attachment 125568 [details] Patch Attachment 125568 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11433158
Created attachment 125649 [details] Fixing some build errors
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.
(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.
> 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.
(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?
(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?
> Maybe I could just move the sweep function out of the header and rename sweepSlowCase to something less related to speed? Sounds good.
> 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 on attachment 125649 [details] Fixing some build errors r=me with the changes we've discussed, and a fixed Mac build.
Committed r107445: <http://trac.webkit.org/changeset/107445>
(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?
I cc-ed Qt JSC experts too, could you check it?
(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.
(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.