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