Bug 194212 - [JSC] Make StaticStringImpl & StaticSymbolImpl actually static
Summary: [JSC] Make StaticStringImpl & StaticSymbolImpl actually static
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
: 195577 (view as bug list)
Depends on: 195641
Blocks:
  Show dependency treegraph
 
Reported: 2019-02-03 22:48 PST by Yusuke Suzuki
Modified: 2019-03-15 14:58 PDT (History)
10 users (show)

See Also:


Attachments
Testing... (1.33 KB, patch)
2019-02-03 22:54 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (1.50 KB, patch)
2019-03-11 15:29 PDT, Yusuke Suzuki
mark.lam: 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 2019-02-03 22:48:15 PST
Let's try it and see the performance impact. It possibly reduces the memory footprint.
Comment 1 Yusuke Suzuki 2019-02-03 22:54:48 PST
Created attachment 361039 [details]
Testing...
Comment 2 Daniel Bates 2019-02-03 23:11:03 PST
This would be the awesome!
Comment 3 Yusuke Suzuki 2019-03-11 15:27:46 PDT
*** Bug 195577 has been marked as a duplicate of this bug. ***
Comment 4 Yusuke Suzuki 2019-03-11 15:29:13 PDT
Created attachment 364300 [details]
Patch
Comment 5 Mark Lam 2019-03-11 15:32:41 PDT
Comment on attachment 364300 [details]
Patch

r=me if this does not regress performance.
Comment 6 Yusuke Suzuki 2019-03-11 15:35:07 PDT
(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!
Comment 7 Yusuke Suzuki 2019-03-11 15:37:48 PDT
Committed r242747: <https://trac.webkit.org/changeset/242747>
Comment 8 Radar WebKit Bug Importer 2019-03-11 15:43:36 PDT
<rdar://problem/48786478>
Comment 9 Darin Adler 2019-03-11 16:05:14 PDT
(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.
Comment 10 Yusuke Suzuki 2019-03-11 19:19:30 PDT
(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.
Comment 11 Yusuke Suzuki 2019-03-12 13:58:52 PDT
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.
Comment 12 WebKit Commit Bot 2019-03-12 14:00:06 PDT
Re-opened since this is blocked by bug 195641
Comment 13 Yusuke Suzuki 2019-03-15 11:23:54 PDT
Maybe, it is a good timing to land it again to see the performance effect!
Comment 14 Simon Fraser (smfr) 2019-03-15 14:29:14 PDT
This seemed to increase binary size quite significantly (and it goes down on the rollout).
Comment 15 Yusuke Suzuki 2019-03-15 14:58:40 PDT
(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).