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

[Chris Lord]

On 2020-12-01 18:21, Geoff Garen wrote:
> 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.

I did consider this, I've also made m_hashAndFlags atomic in the
attached patch. Indeed, tests for concurrent string usage and expected
behaviour would be desirable, I would take that as a given (but it's
worth mentioning).

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

This is a bit of a shame, but understandable. It's a shame that bugs
don't track unresolved dependent bugs so that if we split this into
multiple bugs, at least we wouldn't have to submit rolled-up patches for
EWS testing. I'll just keep them separate in my branch I suppose, and
hopefully I can convince people to look there instead of Bugzilla
sometimes... Yes, performance is definitely a challenge. It's helpful
that Google have already trodden this path, but I'm sure there will be
novel work required.

> 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?

I can't talk about GPU Process, this was something that was mentioned to
me when I was talking about methods of making FontCache thread-safe.

FontCache makes extensive use of AtomString for look-ups and
comparisons. I had a few alternative ideas for making FontCache safe to
use in a Worker, but after discussing them on Slack, it seemed like
making FontCache safe for concurrent access and making String
thread-safe were both desirable for future work. I really hope other
people will chime in here, my personal preference would be to do
something less invasive.

Other ideas I had for making FontCache Worker-safe;
- Add a FontCacheProxy object that calls onto the main-thread FontCache
and blocks (not ideal for performance)
- Make FontCache not rely on static data and have a completely separate
FontCache per-Worker, created on-demand (not ideal for memory usage)
- Make FontCache thread-safe, but do it via introducing a completely
separate thread-safe AtomString type and leave the current one as it is
(I don't have a good grasp of how difficult this would actually be)

Cheers,

Chris

> 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