Bug 76665 – Use copying collector for out-of-line JSObject property storage

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

[Mark Hahnenberg]

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.

[Mark Hahnenberg]

Created attachment 123213 [details]
Patch

[Early Warning System Bot]

Comment on attachment 123213 [details]
Patch

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

[Mark Hahnenberg]

Created attachment 123217 [details]
Bencher results

<1% win on SunSpider, 1.4% win on v8, roughly neutral on kraken.

[Geoffrey Garen]

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".

[Mark Hahnenberg]

(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.

[Mark Hahnenberg]

Created attachment 123251 [details]
Patch

[Mark Hahnenberg]

(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.

[Mark Hahnenberg]

> 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)

[Mark Hahnenberg]

Created attachment 123354 [details]
Patch

[Geoffrey Garen]

Comment on attachment 123354 [details]
Patch

r=me if you post performance numbers

[Mark Hahnenberg]

Created attachment 123654 [details]
More bencher results

[Mark Hahnenberg]

Committed r105676: <http://trac.webkit.org/changeset/105676>

[Luke Macpherson]

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"

[Mark Hahnenberg]

Reopening to attach new patch.

[Mark Hahnenberg]

Created attachment 123787 [details]
New patch that avoids GCC warning

[Geoffrey Garen]

Comment on attachment 123787 [details]
New patch that avoids GCC warning

r=me

[WebKit Review Bot]

Comment on attachment 123787 [details]
New patch that avoids GCC warning

Clearing flags on attachment: 123787

Committed r105816: <http://trac.webkit.org/changeset/105816>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.