Bug 206460 – [bmalloc] Make use of LockHolder strict in some methods of Scavenger

• Component: bmalloc
• Status: RESOLVED FIXED

[Basuke Suzuki]

void Scavenger::runHoldingLock() assume the caller has lock. This rule can be strict by passing LockHolder as other code do.

[Basuke Suzuki]

Created attachment 388119 [details]
PATCH

[Darin Adler]

Comment on attachment 388119 [details]
PATCH

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

Saying review+, but there is something wrong here.

> Source/bmalloc/bmalloc/Scavenger.h:57
> -    void run();
> +    BINLINE void run();

It’s *not* OK to have a public function marked inline that is not defined in the header. This is code that won’t compile in some scenarios, where the code is not part of the appropriate translation unit. Not sure what the situation is. Could be one of these:

a) This member function already gets inlined for callers inside Scavenger.cpp even if we don’t mark it with BINLINE. So we don’t need "always inline" nor do we need to mark this function as inline at all, and we should not make this change.

b) This member function is never called outside the Scavenger class so it can be made private. But it does need to be marked "always inline", so we should keep this BINLINE but also move it from public to private.

c) This member function is called outside the Scavenger class but only inside Scavenger.cpp, so the patch is OK as-is, but we’ll have a problem waiting for us if Ian the future anyone adds any new calls to the function outside Scavenger.cpp.

d ...) Some more complicated situation. Perhaps one with no obvious solution?

I’m guessing it’s (a) and no change should be made here, but I’m not sure.

> Source/bmalloc/bmalloc/Scavenger.h:60
> -    void runSoon();
> +    BINLINE void runSoon();

Ditto.

[Basuke Suzuki]

Comment on attachment 388119 [details]
PATCH

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

>> Source/bmalloc/bmalloc/Scavenger.h:57
>> +    BINLINE void run();
> 
> It’s *not* OK to have a public function marked inline that is not defined in the header. This is code that won’t compile in some scenarios, where the code is not part of the appropriate translation unit. Not sure what the situation is. Could be one of these:
> 
> a) This member function already gets inlined for callers inside Scavenger.cpp even if we don’t mark it with BINLINE. So we don’t need "always inline" nor do we need to mark this function as inline at all, and we should not make this change.
> 
> b) This member function is never called outside the Scavenger class so it can be made private. But it does need to be marked "always inline", so we should keep this BINLINE but also move it from public to private.
> 
> c) This member function is called outside the Scavenger class but only inside Scavenger.cpp, so the patch is OK as-is, but we’ll have a problem waiting for us if Ian the future anyone adds any new calls to the function outside Scavenger.cpp.
> 
> d ...) Some more complicated situation. Perhaps one with no obvious solution?
> 
> I’m guessing it’s (a) and no change should be made here, but I’m not sure.

Ah. Good point. Thanks. As original code doesn't have BINLINE, we should leave this as is, no BINLINE and hope compiler do well for us.

[Basuke Suzuki]

Created attachment 388318 [details]
PATCH

[Basuke Suzuki]

Created attachment 388326 [details]
PATCH

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 388326 [details]:

imported/w3c/web-platform-tests/IndexedDB/fire-error-event-exception.html bug 201481 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 388326 [details]
PATCH

Clearing flags on attachment: 388326

Committed r254871: <https://trac.webkit.org/changeset/254871>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58769352>