Bug 206400 - [WTF] AtomStringTable should be small
Summary: [WTF] AtomStringTable should be small
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-16 22:46 PST by Yusuke Suzuki
Modified: 2020-01-21 14:06 PST (History)
9 users (show)

See Also:


Attachments
Patch (29.69 KB, patch)
2020-01-16 22:48 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (31.31 KB, patch)
2020-01-18 01:00 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (31.28 KB, patch)
2020-01-18 01:01 PST, Yusuke Suzuki
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-01-16 22:46:07 PST
[WTF] AtomStringTable should be small
Comment 1 Yusuke Suzuki 2020-01-16 22:48:29 PST
Created attachment 388014 [details]
Patch

WIP
Comment 2 Yusuke Suzuki 2020-01-16 23:33:57 PST
Putting EWS now, and running A/B tests.
Comment 3 Yusuke Suzuki 2020-01-18 01:00:57 PST
Created attachment 388138 [details]
Patch
Comment 4 Yusuke Suzuki 2020-01-18 01:01:59 PST
Created attachment 388139 [details]
Patch
Comment 5 Sam Weinig 2020-01-18 12:12:30 PST
Comment on attachment 388139 [details]
Patch

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

> Source/WTF/ChangeLog:17
> +        [1]: https://bugs.webkit.org/show_bug.cgi?id=206400

This links to the same bugzilla bug as the one this patch is attached to. I think you meant it to point somewhere else?
Comment 6 Sam Weinig 2020-01-18 12:42:13 PST
How much of a win is this? 

Out of curiosity, do you know how many of the AtomStrings in the table are initialized via ProcessWarming::initializeNames()? It might also be interesting to know how many are lazily added static strings vs. atoms constructed from actual content.  

I ask mostly because if we wanted to reduce the AtomStringTable even more, there were a few ideas I had a while back about how we could consider shrinking it further based on the knowledge that many of the AtomStrings are known at compile time. For instance, one idea was to consider splitting the table in two (likely slowing down the slow case of lookup a bit), and having all the compile time known strings in a const/readonly compacted minimal perfect hash (gperf or the like).
Comment 7 Yusuke Suzuki 2020-01-21 13:31:47 PST
Comment on attachment 388139 [details]
Patch

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

>> Source/WTF/ChangeLog:17
>> +        [1]: https://bugs.webkit.org/show_bug.cgi?id=206400
> 
> This links to the same bugzilla bug as the one this patch is attached to. I think you meant it to point somewhere else?

Oops, fixed https://bugs.webkit.org/show_bug.cgi?id=206469.
Comment 8 Yusuke Suzuki 2020-01-21 13:34:46 PST
(In reply to Sam Weinig from comment #6)
> How much of a win is this? 
> 
> Out of curiosity, do you know how many of the AtomStrings in the table are
> initialized via ProcessWarming::initializeNames()? It might also be
> interesting to know how many are lazily added static strings vs. atoms
> constructed from actual content.  

Not sure, but IIRC, for JavaScriptCore.framework it took 64KB (but my memory is sketchy...).
I have no data about WebCore case. It is possible WebCore allocates even more.

> 
> I ask mostly because if we wanted to reduce the AtomStringTable even more,
> there were a few ideas I had a while back about how we could consider
> shrinking it further based on the knowledge that many of the AtomStrings are
> known at compile time. For instance, one idea was to consider splitting the
> table in two (likely slowing down the slow case of lookup a bit), and having
> all the compile time known strings in a const/readonly compacted minimal
> perfect hash (gperf or the like).

I think it is possible that we can get some memory improvement. And we have a lot of compile-time-known HashMaps allocated at runtime.
I think having compile-time HashMap feature could improve things.
Comment 9 Yusuke Suzuki 2020-01-21 14:05:55 PST
Committed r254881: <https://trac.webkit.org/changeset/254881>
Comment 10 Radar WebKit Bug Importer 2020-01-21 14:06:19 PST
<rdar://problem/58772826>