Bug 70698 – Removed SharedUChar and tightened language around its previous uses

• Component: New Bugs
• Status: RESOLVED FIXED

[Geoffrey Garen]

Removed some String misfeatures

[Geoffrey Garen]

Created attachment 112118 [details]
Patch

[Sam Weinig]

This patch does a bunch of unrelated things and would be easier to review if you split it up.

[David Levin]

Comment on attachment 112118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112118&action=review

> Source/JavaScriptCore/ChangeLog:17
> +        a threading solution by pure inertia.)

Actually this is incorrect on several levels:

1. There is evidence of a speed up: http://trac.webkit.org/changeset/44325 for sharing across JSC and WebCore but maybe you meant across threads. I don't remember if I did that measurement. It was meant for the xml case when the buffer is shared across threads and that buffer may be huge.

2. It shouldn't be happening for small strings (unless someone broke that), but it was specifically tuned to pick a sweet spot for the length at which to copy.

3. The whole point of this feature was due to workers. I had to make UString (ScriptString) shareable across threads. I can't remember why I added the other feature to share string between WebCore and JavaScriptCore but that was a side thing.

Frankly there is something that is slightly broken in this feature due to what someone else did, but it is fixable -- however removing it will not fix things.

So in short please don't do this.

[David Levin]

PS It would be nice to include the original author of the code if you're considering removing it so that you can get the background since you looked at the svn history of it that should have hopefully been apparent.

[Geoffrey Garen]

> PS It would be nice to include the original author of the code if you're considering removing it so that you can get the background since you looked at the svn history of it that should have hopefully been apparent.

I apologize for the oversight -- it looked to me like a bot had auto-CC'd you when I posted the patch, but perhaps I read that wrong.

> Frankly there is something that is slightly broken in this feature due to what someone else did, but it is fixable -- however removing it will not fix things.

I'm more than happy to fix StringImpl's SharedBuffer, or replace it with a solution external to StringImpl, if there's a clear definition of a goal and how to measure it. My instinct to remove it came from seeing that it had some issues, and then seeing that its original reason for being had disappeared. (See below.)

> 1. There is evidence of a speed up: http://trac.webkit.org/changeset/44325 for sharing across JSC and WebCore

This is what I meant when I said, "By my reading, this feature originated as a way to share strings between JavaScriptCore and WebCore, and then morphed into a threading solution by pure inertia."

There was a measurable benefit, recorded in svn, to sharing strings between JavaScriptCore and WebCore. But SharedBuffer doesn't do that anymore. (JavaScriptCore just uses StringImpl directly now, and when I'm done refactoring, they will both use something even more efficient called "Characters" or "PassString".) 

I couldn't find any evidence in svn history that the change to use SharedBuffer to move data across threads was a speedup. (Note that all other classes in WebCore and WebKit, when moving data across threads or processes, just copy the data.)

> It was meant for the xml case when the buffer is shared across threads and that buffer may be huge.

What is "the xml case"? I don't see any mention of it in r44325 or its associated bug.

> 3. The whole point of this feature was due to workers. I had to make UString (ScriptString) shareable across threads. 

I don't see any use of SharedBuffer in workers in TOT. Rather, all worker communication seems to happen via SerializedScriptValue, which owns its own Vector<unsigned char>. Is there still a place where SharedBuffer is the main way to send data to/from a worker?

[David Levin]

(In reply to comment #5)
> > PS It would be nice to include the original author of the code if you're considering removing it so that you can get the background since you looked at the svn history of it that should have hopefully been apparent.
> 
> I apologize for the oversight

No worries.

>  -- it looked to me like a bot had auto-CC'd you 

Luckly :)


> I'm more than happy to fix StringImpl's SharedBuffer, or replace it with a solution external to StringImpl, if there's a clear definition of a goal and how to measure it. My instinct to remove it came from seeing that it had some issues, and then seeing that its original reason for being had disappeared. (See below.)
> 
> > 1. There is evidence of a speed up: http://trac.webkit.org/changeset/44325 for sharing across JSC and WebCore
> 
> This is what I meant when I said, "By my reading, this feature originated as a way to share strings between JavaScriptCore and WebCore, and then morphed into a threading solution by pure inertia."

Well that was just my way of staging the changes because the overall change was involved. 

> 
> There was a measurable benefit, recorded in svn, to sharing strings between JavaScriptCore and WebCore. But SharedBuffer doesn't do that anymore. (JavaScriptCore just uses StringImpl directly now, and when I'm done refactoring, they will both use something even more efficient called "Characters" or "PassString".) 

Oh that's excellent. I didn't realize this was the start of something bigger and you're just removing this to make way for something better!


> 
> > 3. The whole point of this feature was due to workers. I had to make UString (ScriptString) shareable across threads. 
> 
> I don't see any use of SharedBuffer in workers in TOT. Rather, all worker communication seems to happen via SerializedScriptValue, which owns its own Vector<unsigned char>. Is there still a place where SharedBuffer is the main way to send data to/from a worker?

Source/WebCore/platform/CrossThreadCopier.cpp is the place that nearly all threaded messaging happens (including data to and from a worker) so that was where this would be used.

However it was broken due to another perf fix (which did the 1 allocation instead of 2 if you know what I'm referring to) and I haven't had a chance to go back and fix this :( but this removal will make it not possible.

> What is "the xml case"? I don't see any mention of it in r44325 or its associated bug.

It is here: https://bugs.webkit.org/show_bug.cgi?id=30095, but it looks like this has all changed now the two calls don't appear to hop threads anyway which is great.

So after pushing at this a bit more, while it is still available and used for x-cross string transfer, the main use cases are gone and it has been broken for a while now anyway, so I don't object to removing it.

(Part of my initial reaction was seeing a lot of my hard work that I thought did have some benefits called misfeatures. Maybe they were. At the time I thought they had a lot of value but regardless I agree that they have outlived their usefulness. Thanks for baring with me.)

[David Levin]

Comment on attachment 112118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112118&action=review

Please delete http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/CrossThreadRefCounted.h as well since this was the only thing that used it. Hurray! (I really disliked that class.)

And remove the changes that you're doing in the other patch.


You could make the change log more factual and spare my feelings now by just making the title "Remove BufferShared." and really it isn't needed anymore I think it sufficient but whatever you want for the description.

> Source/JavaScriptCore/wtf/text/StringImpl.h:307
> +    PassRefPtr<StringImpl> isolatedCopy() const;

I like the name isolatedCopy. Thanks. (I struggled with naming here and failed and still have an open bug on the naming which you are fixing.)

[David Levin]

*** Bug 31020 has been marked as a duplicate of this bug. ***

[Geoffrey Garen]

> > (JavaScriptCore just uses StringImpl directly now, and when I'm done refactoring, they will both use something even more efficient called "Characters" or "PassString".) 
>
> Oh that's excellent. I didn't realize this was the start of something bigger and you're just removing this to make way for something better!

To give a little more context here: For cases when we get a string from a library or from GC memory, we should be able to use just a character pointer and length to pass the string through most WebCore code without first copying it into a StringImpl. That's what Characters/PassString will be for. When WebCore needs to take ownership, that's when it will copy into a StringImpl.

> (Part of my initial reaction was seeing a lot of my hard work that I thought did have some benefits called misfeatures. Maybe they were. At the time I thought they had a lot of value but regardless I agree that they have outlived their usefulness. Thanks for baring with me.)

Sorry, I shouldn't have said "misfeature". It was once an essential feature! It's just that over time the essential parts migrated into other classes.

> I like the name isolatedCopy. Thanks. (I struggled with naming here and failed and still have an open bug on the naming which you are fixing.)

Thanks.

I was thinking about this, and I think there are two interesting ways we could optimize passing large strings across threads, if need be:

(1) "PassRefPtr<StringImpl> StringImpl::isolatedRelease(PassRefPtr<StringImpl>)". If the underlying StringImpl's refcount were exactly 1, this would steal ownership of the StringImpl and move it across threads without copying. (If the refcount were greater than 1, it would either silently copy or ASSERT -- I can't decide which.) I believe that most code that wants to pass a large string buffer between threads probably deals in newly constructed strings, and can arrange to perform an isolatedRelease().

(2) Characters/PassString. If a "caller" thread wants to pass a string to a "callee" thread, as long as the "callee" thread doesn't need to take ownership of the string, the "caller" can just pass a Characters/PassString object without copying, and then arrange to deref the string once the "callee" thread "returns".

For example, if StringImpl were still the main representation for worker messages, judicious use of PassRefPtr<StringImpl> + StringImpl::isolatedRelease could probably get worker messages to be zero-copy.

I don't have a good sense of which code in WebCore could benefit from this sort of thing, but I'm hoping that you might.

Anyway, I'll post an updated patch with your suggestions included.

[David Levin]

(In reply to comment #9)
> To give a little more context here: For cases when we get a string from a library or from GC memory, we should be able to use just a character pointer and length to pass the string through most WebCore code without first copying it into a StringImpl. That's what Characters/PassString will be for. When WebCore needs to take ownership, that's when it will copy into a StringImpl.

Very nice.

> I was thinking about this, and I think there are two interesting ways we could optimize passing large strings across threads, if need be:
> 
> (1) "PassRefPtr<StringImpl> StringImpl::isolatedRelease(PassRefPtr<StringImpl>)". If the underlying StringImpl's refcount were exactly 1, this would steal ownership of the StringImpl and move it across threads without copying. (If the refcount were greater than 1, it would either silently copy or ASSERT -- I can't decide which.) I believe that most code that wants to pass a large string buffer between threads probably deals in newly constructed strings, and can arrange to perform an isolatedRelease().
> 
> Anyway, I'll post an updated patch with your suggestions included.

Sounds reasonable. The two main use cases are now gone but if anything comes up, I'll make sure to use one of these.

I also need to think about how to encourage #1 to be a natural thing for people to do when message passing -- so we can avoid unnecessary work.

The big thing is deleting CrossThreadRefCounted.h which is confusing and shouldn't be used by anyone now. Hurray!

[Geoffrey Garen]

Created attachment 112263 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 112263 [details]
Patch

Attachment 112263 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10199980

[WebKit Review Bot]

Comment on attachment 112263 [details]
Patch

Attachment 112263 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10208073

[Geoffrey Garen]

Created attachment 112269 [details]
Patch

[David Levin]

Comment on attachment 112269 [details]
Patch

I think this will break the Win build because it has its own list of exports at Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def

but I don't see references to these functions in there. Some thing to watch when you check in.

[WebKit Review Bot]

Comment on attachment 112269 [details]
Patch

Attachment 112269 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10203953

[David Levin]

(In reply to comment #16)
> (From update of attachment 112269 [details])
> Attachment 112269 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10203953

I'm guessing that Source/WebCore/bindings/v8/JavaScriptCallFrame.h needs a 

  #include <wtf/RefCounted.h> 

added to it.

[Geoffrey Garen]

Committed r98316: <http://trac.webkit.org/changeset/98316>

[Ryosuke Niwa]

This patch broke Windows & Chromium builds:
http://build.webkit.org/builders/Windows%20Release%20%28Build%29/builds/22552/steps/compile-webkit/logs/stdio
http://build.webkit.org/builders/Chromium%20Linux%20Release/builds/40093/steps/compile-webkit/logs/stdio