Bug 116848 – It should be possible to record heap operations (both FastMalloc and JSC GC)

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

[Filip Pizlo]

This is the record part of record and replay.

[Filip Pizlo]

Created attachment 203018 [details]
the patch

[Mark Hahnenberg]

Comment on attachment 203018 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203018&action=review

> Source/JavaScriptCore/runtime/Butterfly.h:166
> +    Butterfly* growPropertyStorage(VM&, JSCell* intendedOwner, size_t preCapacity, size_t oldPropertyCapacity, bool hasIndexingHeader, size_t indexingPayloadSizeInBytes, size_t newPropertyCapacity);
> +    Butterfly* growPropertyStorage(VM&, JSCell* intendedOwner, Structure* oldStructure, size_t oldPropertyCapacity, size_t newPropertyCapacity);
> +    Butterfly* growPropertyStorage(VM&, JSCell* intendedOwner, Structure* oldStructure, size_t newPropertyCapacity);
> +    Butterfly* growArrayRight(VM&, JSCell* intendedOwner, Structure* oldStructure, size_t propertyCapacity, bool hadIndexingHeader, size_t oldIndexingPayloadSizeInBytes, size_t newIndexingPayloadSizeInBytes); // Assumes that preCapacity is zero, and asserts as much.
> +    Butterfly* growArrayRight(VM&, JSCell* intendedOwner, Structure*, size_t newIndexingPayloadSizeInBytes);
> +    Butterfly* resizeArray(VM&, JSCell* intendedOwner, size_t propertyCapacity, bool oldHasIndexingHeader, size_t oldIndexingPayloadSizeInBytes, size_t newPreCapacity, bool newHasIndexingHeader, size_t newIndexingPayloadSizeInBytes);
> +    Butterfly* resizeArray(VM&, JSCell* intendedOwner, Structure*, size_t newPreCapacity, size_t newIndexingPayloadSizeInBytes); // Assumes that you're not changing whether or not the object has an indexing header.

These signatures are getting ridiculous.

> Source/WTF/wtf/ConditionalLocker.h:36
> +template <typename T> class ConditionalLocker {

Where do we use this class?

[Filip Pizlo]

(In reply to comment #2)
> (From update of attachment 203018 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203018&action=review
> 
> > Source/JavaScriptCore/runtime/Butterfly.h:166
> > +    Butterfly* growPropertyStorage(VM&, JSCell* intendedOwner, size_t preCapacity, size_t oldPropertyCapacity, bool hasIndexingHeader, size_t indexingPayloadSizeInBytes, size_t newPropertyCapacity);
> > +    Butterfly* growPropertyStorage(VM&, JSCell* intendedOwner, Structure* oldStructure, size_t oldPropertyCapacity, size_t newPropertyCapacity);
> > +    Butterfly* growPropertyStorage(VM&, JSCell* intendedOwner, Structure* oldStructure, size_t newPropertyCapacity);
> > +    Butterfly* growArrayRight(VM&, JSCell* intendedOwner, Structure* oldStructure, size_t propertyCapacity, bool hadIndexingHeader, size_t oldIndexingPayloadSizeInBytes, size_t newIndexingPayloadSizeInBytes); // Assumes that preCapacity is zero, and asserts as much.
> > +    Butterfly* growArrayRight(VM&, JSCell* intendedOwner, Structure*, size_t newIndexingPayloadSizeInBytes);
> > +    Butterfly* resizeArray(VM&, JSCell* intendedOwner, size_t propertyCapacity, bool oldHasIndexingHeader, size_t oldIndexingPayloadSizeInBytes, size_t newPreCapacity, bool newHasIndexingHeader, size_t newIndexingPayloadSizeInBytes);
> > +    Butterfly* resizeArray(VM&, JSCell* intendedOwner, Structure*, size_t newPreCapacity, size_t newIndexingPayloadSizeInBytes); // Assumes that you're not changing whether or not the object has an indexing header.
> 
> These signatures are getting ridiculous.

Word.

Do you have any suggestions for how to shrink them?

> 
> > Source/WTF/wtf/ConditionalLocker.h:36
> > +template <typename T> class ConditionalLocker {
> 
> Where do we use this class?

Ooops!  I'll remove it.

[Mark Hahnenberg]

> > These signatures are getting ridiculous.
> 
> Word.
> 
> Do you have any suggestions for how to shrink them?
> 
Maybe we could use a struct to pass all of the various newSize, oldSize, hasIndexingHeader, etc. stuff? Not necessary for this patch though.

[Filip Pizlo]

Created attachment 203065 [details]
the patch

Removed ConditionalLocker.  I don't know why it was there.

[Filip Pizlo]

(In reply to comment #4)
> > > These signatures are getting ridiculous.
> > 
> > Word.
> > 
> > Do you have any suggestions for how to shrink them?
> > 
> Maybe we could use a struct to pass all of the various newSize, oldSize, hasIndexingHeader, etc. stuff? Not necessary for this patch though.

I wish we could do ML-style records.

What about introducing classes for each of the arguments so that instead of saying:

butterfly->thingy(size + 100, size, someFlag, false, blah);

You say:

butterfly->thingy(NewSize(size + 100), OldSize(size), HasIndexingHeader(someFlag), Whatever(false), Something(blah));

(replace "Whatever" and "Something" with other meaningful terms.)

This could work, but I feel like we ought to have some macro magic for generating these, so that we don't get too much cruft all over the place to just create them.

[Mark Hahnenberg]

Comment on attachment 203065 [details]
the patch

r=me

[Early Warning System Bot]

Comment on attachment 203065 [details]
the patch

Attachment 203065 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/666121

[Early Warning System Bot]

Comment on attachment 203065 [details]
the patch

Attachment 203065 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/672433

[EFL EWS Bot]

Comment on attachment 203065 [details]
the patch

Attachment 203065 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/683318

[EFL EWS Bot]

Comment on attachment 203065 [details]
the patch

Attachment 203065 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/683314

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/150811