Re: [webkit-dev] Commit Authorship on GitHub

[Yusuke Suzuki via webkit-dev]

> On Dec 1, 2020, at 11:57 AM, Jonathan Bedard  wrote:
> 
> 
> 
>> On Dec 1, 2020, at 1:55 PM, Yusuke Suzuki > > wrote:
>> 
>> Hi Jonathan!
>> 
>>> On Dec 1, 2020, at 8:22 AM, Jonathan Bedard via webkit-dev 
>>> mailto:webkit-dev@lists.webkit.org>> wrote:
>>> 
>>> Hello contributors,
>>> 
>>> I am in the process of modifying one of our Git mirrors of the repository 
>>> for permanent use. As part of that modification, I am repairing authorship 
>>> of historical commits based on contributors.json. This effort includes our 
>>> branches and resolving commits attributed to commit-queue but authored by 
>>> contributors. Once this task of rewriting history is completed, I will push 
>>> the new repository to GitHub to replace the broken mirror that currently 
>>> resides there.
>> 
>> Does it mean that https://github.com/WebKit/WebKit 
>>  will become an usual repository (not 
>> GitHub sync-ed mirror repository) which is mirrored by ourselves?
>> Previously, when I tried, GitHub-mirrored repository does not invoke 
>> web-hooks correctly, and it was the reason why I needed to create WKR bots.
>> But if WebKit in GitHub repository becomes an usual repository (while it is 
>> mirrored, it is not mirrored by GitHub side), I think this is a good timing 
>> to setting up GitHub <-> slack integration to put commits into #changes and 
>> retiring WKR bot (while WebKitBot exists).
> 
> It does mean that https://github.com/WebKit/WebKit 
>  will become a normal GitHub repository! 
> That being said, I need to set up the automated syncing before we start using 
> web-hooks.

Cool! Let me know in slack etc. when the repository gets ready. I’ll look into 
GitHub integration for commits and retire WKR feature if GitHub slack 
integration can cover that feature.

-Yusuke


> 
> Jonathan
> 
>> 
>> -Yusuke
>> 
>>> 
>>> Since the new repository will have correctly attributed commits, now is a 
>>> good time to ensure that the email address (or addresses) that you use or 
>>> have used to contribute to WebKit are attached to your GitHub account, 
>>> since this is how GitHub connects a user to their contributions.
>>> 
>>> Also note that GitHub will still just be a mirror for the next few months, 
>>> so there is no requirement to have an account with GitHub yet.
>>> 
>>> Jonathan
>>> ___
>>> 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

Re: [webkit-dev] Commit Authorship on GitHub

[Jonathan Bedard via webkit-dev]

> On Dec 1, 2020, at 1:55 PM, Yusuke Suzuki  wrote:
> 
> Hi Jonathan!
> 
>> On Dec 1, 2020, at 8:22 AM, Jonathan Bedard via webkit-dev 
>> mailto:webkit-dev@lists.webkit.org>> wrote:
>> 
>> Hello contributors,
>> 
>> I am in the process of modifying one of our Git mirrors of the repository 
>> for permanent use. As part of that modification, I am repairing authorship 
>> of historical commits based on contributors.json. This effort includes our 
>> branches and resolving commits attributed to commit-queue but authored by 
>> contributors. Once this task of rewriting history is completed, I will push 
>> the new repository to GitHub to replace the broken mirror that currently 
>> resides there.
> 
> Does it mean that https://github.com/WebKit/WebKit 
>  will become an usual repository (not 
> GitHub sync-ed mirror repository) which is mirrored by ourselves?
> Previously, when I tried, GitHub-mirrored repository does not invoke 
> web-hooks correctly, and it was the reason why I needed to create WKR bots.
> But if WebKit in GitHub repository becomes an usual repository (while it is 
> mirrored, it is not mirrored by GitHub side), I think this is a good timing 
> to setting up GitHub <-> slack integration to put commits into #changes and 
> retiring WKR bot (while WebKitBot exists).

It does mean that https://github.com/WebKit/WebKit 
 will become a normal GitHub repository! That 
being said, I need to set up the automated syncing before we start using 
web-hooks.

Jonathan

> 
> -Yusuke
> 
>> 
>> Since the new repository will have correctly attributed commits, now is a 
>> good time to ensure that the email address (or addresses) that you use or 
>> have used to contribute to WebKit are attached to your GitHub account, since 
>> this is how GitHub connects a user to their contributions.
>> 
>> Also note that GitHub will still just be a mirror for the next few months, 
>> so there is no requirement to have an account with GitHub yet.
>> 
>> Jonathan
>> ___
>> 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

Re: [webkit-dev] Commit Authorship on GitHub

[Yusuke Suzuki via webkit-dev]

Hi Jonathan!

> On Dec 1, 2020, at 8:22 AM, Jonathan Bedard via webkit-dev 
>  wrote:
> 
> Hello contributors,
> 
> I am in the process of modifying one of our Git mirrors of the repository for 
> permanent use. As part of that modification, I am repairing authorship of 
> historical commits based on contributors.json. This effort includes our 
> branches and resolving commits attributed to commit-queue but authored by 
> contributors. Once this task of rewriting history is completed, I will push 
> the new repository to GitHub to replace the broken mirror that currently 
> resides there.

Does it mean that https://github.com/WebKit/WebKit 
 will become an usual repository (not GitHub 
sync-ed mirror repository) which is mirrored by ourselves?
Previously, when I tried, GitHub-mirrored repository does not invoke web-hooks 
correctly, and it was the reason why I needed to create WKR bots.
But if WebKit in GitHub repository becomes an usual repository (while it is 
mirrored, it is not mirrored by GitHub side), I think this is a good timing to 
setting up GitHub <-> slack integration to put commits into #changes and 
retiring WKR bot (while WebKitBot exists).

-Yusuke

> 
> Since the new repository will have correctly attributed commits, now is a 
> good time to ensure that the email address (or addresses) that you use or 
> have used to contribute to WebKit are attached to your GitHub account, since 
> this is how GitHub connects a user to their contributions.
> 
> Also note that GitHub will still just be a mirror for the next few months, so 
> there is no requirement to have an account with GitHub yet.
> 
> Jonathan
> ___
> 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

Re: [webkit-dev] Making WTF::StringImpl and WTF::AtomString thread-safe

[Geoff Garen via webkit-dev]

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

[webkit-dev] Making WTF::StringImpl and WTF::AtomString thread-safe

[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:
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] Commit Authorship on GitHub

[Jonathan Bedard via webkit-dev]

Hello contributors,

I am in the process of modifying one of our Git mirrors of the repository for 
permanent use. As part of that modification, I am repairing authorship of 
historical commits based on contributors.json. This effort includes our 
branches and resolving commits attributed to commit-queue but authored by 
contributors. Once this task of rewriting history is completed, I will push the 
new repository to GitHub to replace the broken mirror that currently resides 
there.

Since the new repository will have correctly attributed commits, now is a good 
time to ensure that the email address (or addresses) that you use or have used 
to contribute to WebKit are attached to your GitHub account, since this is how 
GitHub connects a user to their contributions.

Also note that GitHub will still just be a mirror for the next few months, so 
there is no requirement to have an account with GitHub yet.

Jonathan
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev