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
> 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
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)
>> On Dec 1, 2020, at 9:09 AM, 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:
>> 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