Bug 194212 – [JSC] Make StaticStringImpl & StaticSymbolImpl actually static

• Status: REOPENED
• Component: Web Template Framework

[Yusuke Suzuki]

Let's try it and see the performance impact. It possibly reduces the memory footprint.

[Yusuke Suzuki]

Created attachment 361039 [details]
Testing...

[Daniel Bates]

This would be the awesome!

[Yusuke Suzuki]

*** Bug 195577 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Created attachment 364300 [details]
Patch

[Mark Lam]

Comment on attachment 364300 [details]
Patch

r=me if this does not regress performance.

[Yusuke Suzuki]

(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!

[Yusuke Suzuki]

Committed r242747: <https://trac.webkit.org/changeset/242747>

[Radar WebKit Bug Importer]

<rdar://problem/48786478>

[Darin Adler]

(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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 195641

[Yusuke Suzuki]

Maybe, it is a good timing to land it again to see the performance effect!

[Simon Fraser (smfr)]

This seemed to increase binary size quite significantly (and it goes down on the rollout).

[Yusuke Suzuki]

(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).