| Summary: | [JSC] Make StaticStringImpl & StaticSymbolImpl actually static | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||
| Component: | Web Template Framework | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||
| Status: | REOPENED --- | ||||||||
| Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, commit-queue, darin, dbates, ews-watchlist, mark.lam, simon.fraser, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=185337 | ||||||||
| Bug Depends on: | 195641 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
|
Description
Yusuke Suzuki
2019-02-03 22:48:15 PST
Created attachment 361039 [details]
Testing...
This would be the awesome! *** Bug 195577 has been marked as a duplicate of this bug. *** Created attachment 364300 [details]
Patch
Comment on attachment 364300 [details]
Patch
r=me if this does not regress performance.
(In reply to Mark Lam from comment #5) > Comment on attachment 364300 [details] > Patch > > r=me if this does not regress performance. Yes, let's monitor the bots! Committed r242747: <https://trac.webkit.org/changeset/242747> (In reply to Yusuke Suzuki from comment #0) > Let's try it and see the performance impact. Waiting with bated breath. I would be super surprised if we don’t see performance impact. (In reply to Darin Adler from comment #9) > (In reply to Yusuke Suzuki from comment #0) > > Let's try it and see the performance impact. > > Waiting with bated breath. I would be super surprised if we don’t see > performance impact. Yeah, in A/B test the result seems neutral. And I think it can be neutral since this change does not add additional load / store (isStatic()'s flag resides in ref count number itself). But... I'm not sure, and watching the bots. If we can keep this change, we have a chance to put fully-static-atomic-string-hash-table in front of runtime atomic string table. It seems that this is perf-neutral in iOS, however, it is a bit hard to check the performance difference right now since r242714 causes large difference, and this patch is involved in the patch set including r242714. I think, 1. Once rolling out this patch 2. See the performance difference 3. After r242714's related changes are landed, let's land it again and see performance difference is better. Re-opened since this is blocked by bug 195641 Maybe, it is a good timing to land it again to see the performance effect! This seemed to increase binary size quite significantly (and it goes down on the rollout). (In reply to Simon Fraser (smfr) from comment #14) > This seemed to increase binary size quite significantly (and it goes down on > the rollout). Yes, this is because StringImpl::{ref,deref} code increases. (ReadOnly memory) But if we can avoid all heap allocations for AtomicStringImpls (and other types of WTFString backed by StaticStringImpl) finally, I think we can get net win in dirty memory. (Still, StaticStringImpl includes pointer, so it can be seen as a dirty __DATA memory. But if it is in a shared cache, then it can be a complete ReadOnly memory). |