[webkit-reviews] review granted: [Bug 186422] Experiment: create lots of different malloc zones for easier accounting of memory use : [Attachment 386632] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 186422: Experiment: create lots of different malloc zones for easier
accounting of memory use
https://bugs.webkit.org/show_bug.cgi?id=186422

Attachment 386632: Patch

https://bugs.webkit.org/attachment.cgi?id=386632&action=review




--- Comment #64 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 386632
  --> https://bugs.webkit.org/attachment.cgi?id=386632
Patch

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

> Source/JavaScriptCore/bytecode/AccessCase.h:83
> +    WTF_MAKE_FAST_ALLOCATED_FROM_MALLOC_ZONE(AccessCase);

I don't this should be done in this patch, but it would be good to file a bug
where we just made:
- WTF_MAKE_FAST_ALLOCATED always takes the class name, so in many cases, we
won't need to use this specific macro, and it can automatically flip on malloc
zones when the compile flag is on

> Source/WTF/wtf/FastMalloc.cpp:287
> +#define TRACK_MALLOCS 0

you should talk about this in one of the changelogs.

Also, can we name this TRACK_MALLOC_CALLSTACK or something similar to say it
more specifically is tracking callstack data.

> Source/WTF/wtf/FastMalloc.cpp:293
> +class AvoidRecordingScope {

Should we put this in TLS instead of having a global atomic?

> Source/WTF/wtf/FastMalloc.cpp:379
> +    auto addResult = m_addressMallocSiteData.add(newAddress,
WTFMove(it->value));
> +    ASSERT_UNUSED(addResult, addResult.isNewEntry);
> +
> +    m_addressMallocSiteData.remove(it);

you should only do this when `newAddress != oldAddress`

> Source/WTF/wtf/FastMalloc.cpp:442
> +	       const size_t framesToSkip = 6;

nit: I don't think this is super important to do. But I wonder if we could
somehow programmatically figure out this constant by doing string comparisons
to the top stack frames.