Now that we have this shiny new collector we should use it for any out-of-line JSObject property storage that needs to be allocated.
Created attachment 123213 [details] Patch
Comment on attachment 123213 [details] Patch Attachment 123213 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11247226
Created attachment 123217 [details] Bencher results <1% win on SunSpider, 1.4% win on v8, roughly neutral on kraken.
Comment on attachment 123213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123213&action=review On the right track, but I think this could use another cut. > Source/JavaScriptCore/runtime/JSObject.cpp:108 > + if (storage) { It shouldn't be possible for an object to have a NULL storage pointer. Please remove this test. > Source/JavaScriptCore/runtime/JSObject.cpp:113 > + visitor.copyAndAppend(reinterpret_cast<void**>(&storage), thisObject->structure()->propertyStorageCapacity() * sizeof(WriteBarrierBase<Unknown>), storage->slot(), storageSize); It occurs to me now that it's a little awkward for copyAndAppend to take a number of bytes as its argument -- would be more appropriate to take a JSValue** and a count, since it assumes the contents are JSValue anyway (in order to do marking). > Source/JavaScriptCore/runtime/JSObject.cpp:663 > + newPropertyStorage[i].clear(); Instead of calling clear(), you should call operator new(NotNullTag, void*). That's the efficient and semantically correct C++ way to say "I want a new object in this location".
(In reply to comment #4) > > Source/JavaScriptCore/runtime/JSObject.cpp:113 > > + visitor.copyAndAppend(reinterpret_cast<void**>(&storage), thisObject->structure()->propertyStorageCapacity() * sizeof(WriteBarrierBase<Unknown>), storage->slot(), storageSize); > > It occurs to me now that it's a little awkward for copyAndAppend to take a number of bytes as its argument -- would be more appropriate to take a JSValue** and a count, since it assumes the contents are JSValue anyway (in order to do marking). > This interface is necessary for JSArray because the JSValues are at an offset from the beginning of the ArrayStorage buffer.
Created attachment 123251 [details] Patch
(In reply to comment #5) > (In reply to comment #4) > > > Source/JavaScriptCore/runtime/JSObject.cpp:113 > > > + visitor.copyAndAppend(reinterpret_cast<void**>(&storage), thisObject->structure()->propertyStorageCapacity() * sizeof(WriteBarrierBase<Unknown>), storage->slot(), storageSize); > > > > It occurs to me now that it's a little awkward for copyAndAppend to take a number of bytes as its argument -- would be more appropriate to take a JSValue** and a count, since it assumes the contents are JSValue anyway (in order to do marking). > > > > This interface is necessary for JSArray because the JSValues are at an offset from the beginning of the ArrayStorage buffer. Perhaps we could add a convenience wrapper function that handles the case where we just have an array of JSValues.
> Perhaps we could add a convenience wrapper function that handles the case where we just have an array of JSValues. Actually, there are two ways we could do this. The structure keeps track of both the (used) size and the capacity of the property storage. We need to pass in the capacity so that it copies the entire backing store, but we also could use the size to determine how many JSValues to append. If we stick with this admittedly ugly version of copyAndAppend, then we will be able to get rid of the part where we have to clear any unused memory. Or we could do the slightly cleaner syntax and clear the unused memory and just always pass in the capacity rather than the size to copyAndAppend(JSValue**, size_t)
Created attachment 123354 [details] Patch
Comment on attachment 123354 [details] Patch r=me if you post performance numbers
Created attachment 123654 [details] More bencher results
Committed r105676: <http://trac.webkit.org/changeset/105676>
I rolled this out in http://trac.webkit.org/changeset/105682 as it was breaking the build due to compile warnings and I wasn't seeing any responses on IRC. Admittedly webkit-rollout surprised me by committing the rollout automatically - I would have preferred to confirm and contact first, but that's how it happened. FYI, the build errors were of the form: "JSObject.cpp:108: warning: dereferencing type-punned pointer will break strict-aliasing rules"
Reopening to attach new patch.
Created attachment 123787 [details] New patch that avoids GCC warning
Comment on attachment 123787 [details] New patch that avoids GCC warning r=me
Comment on attachment 123787 [details] New patch that avoids GCC warning Clearing flags on attachment: 123787 Committed r105816: <http://trac.webkit.org/changeset/105816>
All reviewed patches have been landed. Closing bug.