Bug 201555 - Revert to pre-r243144 scavenging behavior for macOS
Summary: Revert to pre-r243144 scavenging behavior for macOS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: bmalloc (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-06 13:23 PDT by Michael Saboff
Modified: 2019-09-09 15:38 PDT (History)
2 users (show)

See Also:


Attachments
Patch (22.92 KB, patch)
2019-09-06 13:27 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff
Updated patch with "Scavenger::verbose" flag set to false (22.74 KB, patch)
2019-09-06 13:42 PDT, Michael Saboff
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2019-09-06 13:23:24 PDT
Various tests seem to indicate that the code before r243144 is actually better for Macs.
Comment 1 Michael Saboff 2019-09-06 13:24:04 PDT
<rdar://problem/53995557>
Comment 2 Michael Saboff 2019-09-06 13:27:23 PDT
Created attachment 378225 [details]
Patch
Comment 3 Michael Saboff 2019-09-06 13:42:19 PDT
Created attachment 378226 [details]
Updated patch with "Scavenger::verbose" flag set to false
Comment 4 Saam Barati 2019-09-06 14:05:45 PDT
(In reply to Michael Saboff from comment #0)
> Various tests seem to indicate that the code before r243144 is actually
> better for Macs.

Which tests?
Comment 5 Saam Barati 2019-09-06 14:08:42 PDT
Comment on attachment 378226 [details]
Updated patch with "Scavenger::verbose" flag set to false

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

Why so much special casing on platform(MAC) vs not? Was that really in the original code?

> Source/bmalloc/ChangeLog:7
> +

you should say what and why

> Source/bmalloc/bmalloc/LargeRange.h:170
> +#if !BPLATFORM(MAC)
> +        , mergedUsedSinceLastScavenge
> +#endif

do we still use this bit? Why just on iOS? Seems weird

> Source/bmalloc/bmalloc/Scavenger.cpp:226
> +#if BPLATFORM(MAC)
> +                PerProcess<PerHeapKind<Heap>>::get()->at(i).scavenge(lock, decommitter);
> +#else
>                  PerProcess<PerHeapKind<Heap>>::get()->at(i).scavenge(lock, decommitter, deferredDecommits);
> +#endif

what's different about these two code paths per platform?
Comment 6 Michael Saboff 2019-09-06 15:25:55 PDT
Comment on attachment 378226 [details]
Updated patch with "Scavenger::verbose" flag set to false

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

>> Source/bmalloc/ChangeLog:7
>> +
> 
> you should say what and why

I'll add "The change in r243144 regressed internal power metrics for some Mac models."

>> Source/bmalloc/bmalloc/LargeRange.h:170
>> +#endif
> 
> do we still use this bit? Why just on iOS? Seems weird

The used bits are integral to the current code and required for the non-Mac code paths.

>> Source/bmalloc/bmalloc/Scavenger.cpp:226
>> +#endif
> 
> what's different about these two code paths per platform?

The extra "deferredDecommits" reference parameter, which is used in the non-Mac code below.
Comment 7 Saam Barati 2019-09-06 16:20:59 PDT
Comment on attachment 378226 [details]
Updated patch with "Scavenger::verbose" flag set to false

r=me
Comment 8 Michael Saboff 2019-09-09 15:38:43 PDT
Committed r249670: <https://trac.webkit.org/changeset/249670>