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.

Were you able to quantify anything additional about the performance regression on Intel, for instance, if there were any types of String usage that were particularly hard hit? If not (or you haven’t tried), do you think there are any metrics we should look into gathering that might help pin point where things could maybe be optimized? Ideally we would be able to identify places where we unnecessarily ref-deref StringImpls where either a move or judicious use of StringView would work just as well. 

I didn’t debug deeply. Maybe we’ll be able to reduce refcount changes. Maybe not.

The Blink document (linked in the bugzilla bug) suggests they identified some mitigations, but I am not clear they will be representative for us today.

It’s not obvious to me that what we’re doing and what Blink are doing are analogous. One difference is that we use the String class in our JavaScript engine. Another difference is that we have a no regression policy on benchmarks.

Thanks,
Geoff


Putting on my old bindings hat, one area that might be able to get some wins here is how Strings (and other ref counted things, but let’s stay focused) are passed from the JS bindings layer to the DOM. In most cases Strings should be able to be moved into the DOM without refcount churn, but since many of our functions take `const String&` or `const AtomString&` we lose out.

- Sam


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