Summary: | It should be possible to record heap operations (both FastMalloc and JSC GC) | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||
Component: | JavaScriptCore | Assignee: | 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
Filip Pizlo
2013-05-27 17:04:43 PDT
Created attachment 203018 [details]
the patch
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? (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. > > 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.
Created attachment 203065 [details]
the patch
Removed ConditionalLocker. I don't know why it was there.
(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 on attachment 203065 [details]
the patch
r=me
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 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 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 on attachment 203065 [details] the patch Attachment 203065 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/683314 Landed in http://trac.webkit.org/changeset/150811 |