RESOLVED FIXED 64745
JSC GC lazy sweep does not inline the common cases of cell destruction
https://bugs.webkit.org/show_bug.cgi?id=64745
Summary JSC GC lazy sweep does not inline the common cases of cell destruction
Filip Pizlo
Reported 2011-07-18 13:18:35 PDT
Most objects in the heap are instances of JSFinalObject, which has a relatively straight-forward destructor. This destructor's common case is to do nothing: the property storage is not inline, so there is nothing to destroy. However, the lazy sweep will currently always make a slow virtual call because it doesn't special-case the common cases.
Attachments
the patch (2.12 KB, patch)
2011-07-18 13:27 PDT, Filip Pizlo
oliver: review-
oliver: commit-queue-
the patch (fix review) (1.79 KB, patch)
2011-07-18 14:33 PDT, Filip Pizlo
no flags
Filip Pizlo
Comment 1 2011-07-18 13:27:16 PDT
Created attachment 101197 [details] the patch
Oliver Hunt
Comment 2 2011-07-18 13:53:48 PDT
Comment on attachment 101197 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=101197&action=review r- :( > Source/JavaScriptCore/heap/MarkedBlock.cpp:96 > + if (*reinterpret_cast<void**>(cell) == jsFinalObjectVPtr) { use cell->vptr(); > Source/JavaScriptCore/heap/MarkedBlock.cpp:99 > + if (!object->isUsingInlineStorage()) > + delete [] object->m_propertyStorage; object->JSFinalObject::~JSFinalObject() didn't work?
Filip Pizlo
Comment 3 2011-07-18 13:59:07 PDT
(In reply to comment #2) > (From update of attachment 101197 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101197&action=review > > r- :( > > > Source/JavaScriptCore/heap/MarkedBlock.cpp:96 > > + if (*reinterpret_cast<void**>(cell) == jsFinalObjectVPtr) { > > use cell->vptr(); Ah, that's handy! > > Source/JavaScriptCore/heap/MarkedBlock.cpp:99 > > + if (!object->isUsingInlineStorage()) > > + delete [] object->m_propertyStorage; > > object->JSFinalObject::~JSFinalObject() didn't work? I tried object->~JSFinalObject() but that didn't give a speed-up. I forgot the object->JSFinalObject::~JSFinalObject syntax. That works great. Doing some quick tests and will resubmit shortly...
Filip Pizlo
Comment 4 2011-07-18 14:33:43 PDT
Created attachment 101205 [details] the patch (fix review)
Darin Adler
Comment 5 2011-07-18 14:41:07 PDT
Comment on attachment 101205 [details] the patch (fix review) View in context: https://bugs.webkit.org/attachment.cgi?id=101205&action=review > Source/JavaScriptCore/heap/MarkedBlock.cpp:98 > + JSFinalObject* object = reinterpret_cast<JSFinalObject*>(cell); > + object->JSFinalObject::~JSFinalObject(); I would have done this as a one liner: reinterpret_cast<JSFinalObject*>(cell)->object->JSFinalObject::~JSFinalObject() I also think it deserves a “why” comment, stating this is a special case for speed.
WebKit Review Bot
Comment 6 2011-07-18 15:56:00 PDT
Comment on attachment 101205 [details] the patch (fix review) Clearing flags on attachment: 101205 Committed r91218: <http://trac.webkit.org/changeset/91218>
WebKit Review Bot
Comment 7 2011-07-18 15:56:04 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.