| Summary: | [WTF] AtomStringTable should be small | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||
| Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, dbates, esprehn+autocc, ews-watchlist, kangil.han, sam, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Yusuke Suzuki
2020-01-16 22:46:07 PST
Created attachment 388014 [details]
Patch
WIP
Putting EWS now, and running A/B tests. Created attachment 388138 [details]
Patch
Created attachment 388139 [details]
Patch
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? 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 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. (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. Committed r254881: <https://trac.webkit.org/changeset/254881> |