Bug 116848

Summary: It should be possible to record heap operations (both FastMalloc and JSC GC)
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, benjamin, cmarcelo, commit-queue, eflews.bot, ggaren, gyuyoung.kim, mark.lam, mhahnenberg, msaboff, oliver, philn, sam, webkit-ews, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 116763    
Attachments:
Description Flags
the patch
none
the patch mhahnenberg: review+, webkit-ews: commit-queue-

Description Filip Pizlo 2013-05-27 17:04:43 PDT
This is the record part of record and replay.
Comment 1 Filip Pizlo 2013-05-27 17:07:22 PDT
Created attachment 203018 [details]
the patch
Comment 2 Mark Hahnenberg 2013-05-28 10:21:34 PDT
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?
Comment 3 Filip Pizlo 2013-05-28 10:22:28 PDT
(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.
Comment 4 Mark Hahnenberg 2013-05-28 10:24:23 PDT
> > 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.
Comment 5 Filip Pizlo 2013-05-28 10:25:03 PDT
Created attachment 203065 [details]
the patch

Removed ConditionalLocker.  I don't know why it was there.
Comment 6 Filip Pizlo 2013-05-28 10:28:28 PDT
(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.
Comment 7 Mark Hahnenberg 2013-05-28 10:33:41 PDT
Comment on attachment 203065 [details]
the patch

r=me
Comment 8 Early Warning System Bot 2013-05-28 10:35:04 PDT
Comment on attachment 203065 [details]
the patch

Attachment 203065 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/666121
Comment 9 Early Warning System Bot 2013-05-28 10:37:14 PDT
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
Comment 10 EFL EWS Bot 2013-05-28 11:00:38 PDT
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
Comment 11 EFL EWS Bot 2013-05-28 11:07:42 PDT
Comment on attachment 203065 [details]
the patch

Attachment 203065 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/683314
Comment 12 Filip Pizlo 2013-05-28 11:18:22 PDT
Landed in http://trac.webkit.org/changeset/150811