Bug 226149 - Use singleton thunks for virtual calls.
Summary: Use singleton thunks for virtual calls.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-22 18:02 PDT by Mark Lam
Modified: 2021-05-23 00:12 PDT (History)
7 users (show)

See Also:


Attachments
proposed patch. (12.38 KB, patch)
2021-05-22 18:21 PDT, Mark Lam
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
proposed patch. (12.46 KB, patch)
2021-05-22 21:55 PDT, Mark Lam
ysuzuki: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2021-05-22 18:02:26 PDT
The pre-existing implementation emits a unique thunk for each virtual call site.  This turns out to be very wasteful in terms of memory.  For example, Speedometer2 ends up generating ~16M of virtualFor thunks.  Switching to re-useable singleton thunks, saves that 16M, and also appears to improve Speedometer2 performance by 1.012x.
Comment 1 Radar WebKit Bug Importer 2021-05-22 18:02:55 PDT
<rdar://problem/78357604>
Comment 2 Mark Lam 2021-05-22 18:03:41 PDT
Forgot to say, the 1.012x was measured on a M1 Mac.
Comment 3 Mark Lam 2021-05-22 18:21:53 PDT
Created attachment 429445 [details]
proposed patch.

Let's try this on the EWS.
Comment 4 Mark Lam 2021-05-22 21:55:42 PDT
Created attachment 429457 [details]
proposed patch.
Comment 5 Yusuke Suzuki 2021-05-22 22:32:15 PDT
Comment on attachment 429457 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/ChangeLog:13
> +        2. Introduce Options::useUniqueVirtualThunks() to allow unique thunks to be
> +           generated for testing and comparisons.  Options::useUniqueVirtualThunks() is
> +           false by default.

Maybe this flag is not necessary.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:275
> +    bool isTailCall = (mode == CallMode::Tail);

() is not necessary.
Comment 6 Mark Lam 2021-05-22 23:02:45 PDT
Comment on attachment 429457 [details]
proposed patch.

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

>> Source/JavaScriptCore/ChangeLog:13
>> +           false by default.
> 
> Maybe this flag is not necessary.

I suppose we can always add it later if a need arises.  I'll remove it.

>> Source/JavaScriptCore/jit/ThunkGenerators.cpp:275
>> +    bool isTailCall = (mode == CallMode::Tail);
> 
> () is not necessary.

Will fix.
Comment 7 Mark Lam 2021-05-22 23:24:30 PDT
Thanks for the review.  Landed in r277929: <http://trac.webkit.org/r277929>.