Bug 174973 – Use one Scavenger thread for all Heaps

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

[Filip Pizlo]

...

[Filip Pizlo]

Created attachment 322297 [details]
work in progress

I'm going to get rid of AsyncTask since it's only used for scavenging, and make Scavenger do the work of AsyncTask.  It'll also do the work of scheduling the scavenger (like the scavenger bytes count, and the growing flag).  Then all Heaps will use it.

[Filip Pizlo]

Created attachment 322300 [details]
it is written

I haven't tried it yet though.

I want to first see if I can make bmalloc's mutex adaptive.

[Filip Pizlo]

Created attachment 322957 [details]
the patch

[Build Bot]

Attachment 322957 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/Scavenger.cpp:139:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/bmalloc/bmalloc/Scavenger.cpp:144:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Comment on attachment 322957 [details]
the patch

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

r=me after a few fixes.

> Source/bmalloc/bmalloc/Scavenger.cpp:78
> +    m_isGrowing = true;

Can you call it "m_isProbablyGrowing" or something?

> Source/bmalloc/bmalloc/Scavenger.cpp:87
> +void Scavenger::scheduleIfUnderMemoryPressureHoldingLock(size_t bytes)

Shouldn't you pass in the lock here, to make sure it's actually holding it? Then the name can be shorter.

> Source/bmalloc/bmalloc/Scavenger.cpp:102
> +    runHoldingLock();

Ditto.

> Source/bmalloc/bmalloc/Scavenger.cpp:139
> +    auto truth = [] { return true; };

Wat?

[Filip Pizlo]

(In reply to JF Bastien from comment #5)
> Comment on attachment 322957 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322957&action=review
> 
> r=me after a few fixes.
> 
> > Source/bmalloc/bmalloc/Scavenger.cpp:78
> > +    m_isGrowing = true;
> 
> Can you call it "m_isProbablyGrowing" or something?

Good idea.  That is a more accurate name.

> 
> > Source/bmalloc/bmalloc/Scavenger.cpp:87
> > +void Scavenger::scheduleIfUnderMemoryPressureHoldingLock(size_t bytes)
> 
> Shouldn't you pass in the lock here, to make sure it's actually holding it?
> Then the name can be shorter.

I can't because C++.  Some things in Scavenger use lock_guard, other things use unique_lock.  So there is no single type I could pass in.  Therefore, rather than trying to get too clever, I just used the "HoldingLock" suffix in the same way I would have ordinarily used "const AbstractLocker&" in things that sit above WTF.

> 
> > Source/bmalloc/bmalloc/Scavenger.cpp:102
> > +    runHoldingLock();
> 
> Ditto.
> 
> > Source/bmalloc/bmalloc/Scavenger.cpp:139
> > +    auto truth = [] { return true; };
> 
> Wat?

bmalloc does not have NO_RETURN macros.  I really hate those macros, but we have some warnings set that force us to use them.  Rather than do all that, I just made sure that the while loop would look like it might return.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/222982/webkit

[Radar WebKit Bug Importer]

<rdar://problem/34857295>