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@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@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev