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

2020-12-02 Thread Chris Lord via webkit-dev
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 
>>  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 en

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

2020-12-01 Thread Chris Lord via webkit-dev
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@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev