Bug 203121 - [bmalloc] Bug fix for wait time when it's in mini mode.
Summary: [bmalloc] Bug fix for wait time when it's in mini mode.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: bmalloc (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-10-17 14:44 PDT by Basuke Suzuki
Modified: 2019-11-07 17:29 PST (History)
6 users (show)

See Also:


Attachments
PATCH (1.47 KB, patch)
2019-10-17 14:57 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (2.40 KB, patch)
2019-11-06 19:24 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (2.44 KB, patch)
2019-11-06 19:25 PST, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2019-10-17 14:44:39 PDT
m_waitTime is never changed on mini mode since https://trac.webkit.org/changeset/243144/webkit
Comment 1 Basuke Suzuki 2019-10-17 14:57:26 PDT
Created attachment 381232 [details]
PATCH
Comment 2 Basuke Suzuki 2019-10-17 14:57:51 PDT
Simple bug fix.
Comment 3 Basuke Suzuki 2019-10-17 15:01:37 PDT
Oh, there's no watchOS EWS...
Comment 4 Yusuke Suzuki 2019-10-17 16:05:45 PDT
Comment on attachment 381232 [details]
PATCH

r=me. Yes, this is apparently not intended behavior.
Comment 5 Basuke Suzuki 2019-10-17 16:20:42 PDT
Thanks!
Comment 6 WebKit Commit Bot 2019-10-17 17:02:17 PDT
Comment on attachment 381232 [details]
PATCH

Clearing flags on attachment: 381232

Committed r251268: <https://trac.webkit.org/changeset/251268>
Comment 7 WebKit Commit Bot 2019-10-17 17:02:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2019-10-17 17:03:36 PDT
<rdar://problem/56391220>
Comment 9 Yusuke Suzuki 2019-11-06 14:32:58 PST
Internally, we found that this behavior is making RAMification 1% regressed.
And this is because,

1. Due to this accidental behavior, we are always using 10ms for the time interval.
2. And this works well for mini-mode.

For now, I'll roll-out this change and keep mini-mode scavenging period 10ms. And another thing I would like to see is that the memory number w/ system malloc.
Our bmalloc is focusing on performance, while system malloc does good thing for memory footprint in sacrifice of throughput. The purpose of Mini mode is making memory footprint small w/ small throughput loss, and this policy matches well to the system malloc. It would be possible that we could get some good memory footprint further by switching to system malloc by specifying Malloc=1 environment option.

So, for now, I'll roll out this patch, (or, just removing adjusting part for mini-mode too).
Comment 10 Yusuke Suzuki 2019-11-06 15:58:10 PST
Committed r252157: <https://trac.webkit.org/changeset/252157>
Comment 11 Saam Barati 2019-11-06 17:48:07 PST
(In reply to Yusuke Suzuki from comment #10)
> Committed r252157: <https://trac.webkit.org/changeset/252157>

why roll out this code? At least we should make the code sensible instead of updating a local variable which is never used
Comment 12 Yusuke Suzuki 2019-11-06 18:42:33 PST
(In reply to Saam Barati from comment #11)
> (In reply to Yusuke Suzuki from comment #10)
> > Committed r252157: <https://trac.webkit.org/changeset/252157>
> 
> why roll out this code? At least we should make the code sensible instead of
> updating a local variable which is never used

Because of RAMification regression, we decide that the old broken behavior (always using 10ms) is the safest option for now. We will add a patch that is removing adjusting part for mini-mode in a separate patch.
Comment 13 Yusuke Suzuki 2019-11-06 18:43:06 PST
(In reply to Saam Barati from comment #11)
> (In reply to Yusuke Suzuki from comment #10)
> > Committed r252157: <https://trac.webkit.org/changeset/252157>
> 
> why roll out this code? At least we should make the code sensible instead of
> updating a local variable which is never used

See the ChangeLog of roll-out.

>  We should clean up to make this bug's behavior default. And we should look for a bit larger interval here
Comment 14 Yusuke Suzuki 2019-11-06 19:24:12 PST
Reopening to attach new patch.
Comment 15 Yusuke Suzuki 2019-11-06 19:24:13 PST
Created attachment 383012 [details]
Patch
Comment 16 Yusuke Suzuki 2019-11-06 19:25:30 PST
Created attachment 383013 [details]
Patch
Comment 17 Saam Barati 2019-11-07 11:11:16 PST
Comment on attachment 383013 [details]
Patch

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

r=me

> Source/bmalloc/bmalloc/Scavenger.cpp:-504
> -        if (m_isInMiniMode) {
> -            timeSpentScavenging *= 50;
> -            newWaitTime = std::chrono::duration_cast<std::chrono::milliseconds>(timeSpentScavenging);
> -            newWaitTime = std::min(std::max(newWaitTime, std::chrono::milliseconds(25)), std::chrono::milliseconds(500));
> -        } else {

Can we file a FIXME?
Comment 18 Yusuke Suzuki 2019-11-07 17:29:14 PST
Committed r252224: <https://trac.webkit.org/changeset/252224>