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