Bug 201555 – Revert to pre-r243144 scavenging behavior for macOS

• Status: RESOLVED
• Resolution: FIXED
• Component: bmalloc

[Michael Saboff]

Various tests seem to indicate that the code before r243144 is actually better for Macs.

[Michael Saboff]

<rdar://problem/53995557>

[Michael Saboff]

Created attachment 378225 [details]
Patch

[Michael Saboff]

Created attachment 378226 [details]
Updated patch with "Scavenger::verbose" flag set to false

[Saam Barati]

(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?

[Saam Barati]

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?

[Michael Saboff]

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.

[Saam Barati]

Comment on attachment 378226 [details]
Updated patch with "Scavenger::verbose" flag set to false

r=me

[Michael Saboff]

Committed r249670: <https://trac.webkit.org/changeset/249670>