[webkit-dev] Making WTF::StringImpl and WTF::AtomString thread-safe

[Geoff Garen]

A few thoughts here:

1. It’s not just ref counting. 

To make String thread-safe, you also need to address all other data members. That means all state in m_hashAndFlags, including the 8bit/16bit state.

It appears that your testing strategy did not reveal this point so far; so, you probably need to expand your plan for unit testing concurrent access to a string, with a focus on writing tests that fail with the current implementation.

2. Performance is a challenge.

I’ve tested making String thread-safe pretty extensively. On modern Apple Silicon, it’s free. But on Intel chips, it’s very expensive.

Because it’s so expensive, and because we have a no regression policy for performance, I don’t think there’s a way to land this change in pieces. It has to be a mega-patch, so we can test its performance as a whole.

3. I’m surprised by the premise that thread-safe String is a requirement for FontCache and/or the GPU Process.

It’s certainly a false premise that there’s consensus on this premise, since I do not agree.

Can you share some problem statements regarding FontCache and/or the GPU Process that explain the problem we’re trying to solve?

Thanks,
Geoff

> On Dec 1, 2020, at 9:09 AM, Chris Lord via webkit-dev <webkit-dev at lists.webkit.org> wrote:
> 
> Hi all,
> 
> As part of the work for making FontCache thread-safe, it's necessary for
> there to be a thread-safe AtomString. After discussion, it seems that a
> thread-safe StringImpl is generally desirable and GPUProcess also has a
> need of it. I've filed a bug to track this work:
> https://bugs.webkit.org/show_bug.cgi?id=219285
> 
> Google have already done this for Blink and there's a nice plan and lots
> of discussion to read. Their plan document is linked in the bug. I think
> we'd be well-served by taking broadly the same approach, which is to
> make ref-counting on StringImpl atomic and to guard AtomStringTable
> access with a lock.
> 
> Making String thread-safe has implications, of course, and I'd like to
> open discussion on this - Making ref-counting atomic on StringImpl has a
> significant, negative impact on the expense of ref and deref operations.
> I'm interested in discussing how we should approach this in terms of
> tracking the work in Bugzilla and how to go about landing it. Perhaps
> people also have alternative ideas?
> 
> On the bug is a first-run at implementing the above approach, currently
> minus the follow-up of everywhere taking into consideration that
> String/AtomString are now thread-safe. The impact on StringImpl
> ref/deref performance has it running on my Xeon desktop machine at about
> 30-50% of non-atomic ref/deref performance. Speedometer 2.0 takes a 1-8%
> hit considering error margins, but I'm fairly certain it's mostly on the
> higher end of that and I've not run enough iterations just yet.
> Jetstream 1.1 seems practically unaffected, I can't run 2.0 with or
> without the patch, it appears to hang the browser on the bomb-workers
> test (at least if it completes, it's not in a reasonable time-frame). I
> would guess that results may vary wildly depending on platform and
> available atomic access primitives. As one might expect, the impact is
> far less on a debug build.
> 
> I think the initial patch of making it thread-safe vs. the follow-up of
> altering various areas to take it into account could/should be split,
> but I assume we'd want to land them at the same time. This is cumbersome
> with how WebKit Bugzilla currently works and I'd like to hear what
> people think and how similar changes have been made in the past.
> 
> Thoughts?
> 
> Regards,
> 
> Chris
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev