Bug 27655 – [v8] cache v8 strings when converting from webcore string to v8 string

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[anton muhin]

Let's not create many external v8 strings for the same WebCore::StringImpl

[anton muhin]

Created attachment 33453 [details]
Initial version

[David Levin]

Comment on attachment 33453 [details]
Initial version

Just a few things to clean up.

> Index: third_party/WebKit/WebCore/bindings/v8/V8Binding.cpp

> +HashMap<StringImpl*, v8::String*> stringCache;

Better to use DEFINE_STATE_LOCAL.  (Then it is allocated on demand instead of at start up and it won't have to get deleted on shutdown. You could make some statically linked function to expose this.)

> +
> +static void cachedStringCallback(v8::Persistent<v8::Value> wrapper, void* parameter)
> +{

Assert that this is only called on the main thread (since HashMap isn't threadsafe).

> +    StringImpl* stringImpl = static_cast<StringImpl*>(parameter);
> +    ASSERT(stringCache.contains(stringImpl));
> +    stringCache.remove(stringImpl);
> +    wrapper.Dispose();
>  }
>  
>  v8::Local<v8::String> v8ExternalString(const String& string)

A "to" prefix is usually used on conversion functions like this.  (Or perhaps you want findCachedV8String?)

>  {

Assert that this is only called on the main thread (since HashMap isn't threadsafe). (As long as, that's true.)

> +
> +    StringImpl* stringImpl = string.impl();
> +    v8::String* s = stringCache.get(stringImpl);
> +    if (s) return v8::Local<v8::String>(s);

Avoid abbreviations:
s/s/cachedV8String/

[anton muhin]

David, thanks a lot for review!  I would address your comments on Monday.  For now I'd be really glad to hear your (and all others) opinion on threading stuff.  I didn't mentioned when posting a patch (sorry), but actually I know too little to implement it correctly.  First of all, I'd expect worker threads to access this code (I'm not 100% sure, but I'd rather not hard code this assumption).  I don't know if it's safe to use v8 string from different threads.  If it's the case, I'd only protect the cache with a mutex.  If it's not it looks like we need thread locals caches.  But I'd be glad to hear from people with more experience in DOM bindings threading issues.

(In reply to comment #2)
> (From update of attachment 33453 [details])
> Just a few things to clean up.
> 
> > Index: third_party/WebKit/WebCore/bindings/v8/V8Binding.cpp
> 
> > +HashMap<StringImpl*, v8::String*> stringCache;
> 
> Better to use DEFINE_STATE_LOCAL.  (Then it is allocated on demand instead of
> at start up and it won't have to get deleted on shutdown. You could make some
> statically linked function to expose this.)
> 
> > +
> > +static void cachedStringCallback(v8::Persistent<v8::Value> wrapper, void* parameter)
> > +{
> 
> Assert that this is only called on the main thread (since HashMap isn't
> threadsafe).
> 
> > +    StringImpl* stringImpl = static_cast<StringImpl*>(parameter);
> > +    ASSERT(stringCache.contains(stringImpl));
> > +    stringCache.remove(stringImpl);
> > +    wrapper.Dispose();
> >  }
> >  
> >  v8::Local<v8::String> v8ExternalString(const String& string)
> 
> A "to" prefix is usually used on conversion functions like this.  (Or perhaps
> you want findCachedV8String?)
> 
> >  {
> 
> Assert that this is only called on the main thread (since HashMap isn't
> threadsafe). (As long as, that's true.)
> 
> > +
> > +    StringImpl* stringImpl = string.impl();
> > +    v8::String* s = stringCache.get(stringImpl);
> > +    if (s) return v8::Local<v8::String>(s);
> 
> Avoid abbreviations:
> s/s/cachedV8String/

[David Levin]

> I don't know if it's safe to use v8 string from different threads.

I don't know about v8 strings, but StringImpl is certainly not safe for use on multiple threads.

>  If it's the case, I'd only protect the cache with a mutex. 

That should work for the HashMap (but not the StringImpl in it).

> If it's not, it looks like we need thread locals caches.

You may want to add to WebCore/platform/ThreadGlobalData.h


fyi, I've not had time to look into it, but the function v8ValueToWebCoreString in V8Binding.cpp has similar concerns due to its use of AtomicString which may not be used across multiple threads.  (I don't know if this can be used from workers but if it can be, then it should be fixed as well.)

[anton muhin]

Created attachment 33619 [details]
Next iteration

[anton muhin]

David, all, sorry for delay

(In reply to comment #2)
> (From update of attachment 33453 [details])
> Just a few things to clean up.
> 
> > Index: third_party/WebKit/WebCore/bindings/v8/V8Binding.cpp
> 
> > +HashMap<StringImpl*, v8::String*> stringCache;
> 
> Better to use DEFINE_STATE_LOCAL.  (Then it is allocated on demand instead of
> at start up and it won't have to get deleted on shutdown. You could make some
> statically linked function to expose this.)
> 

Done.  I just wanted to keep the code for main thread as fast as possible.

> > +
> > +static void cachedStringCallback(v8::Persistent<v8::Value> wrapper, void* parameter)
> > +{
> 
> Assert that this is only called on the main thread (since HashMap isn't
> threadsafe).

Now not applicable as I have (hopefully) proper support for several threads.

> 
> > +    StringImpl* stringImpl = static_cast<StringImpl*>(parameter);
> > +    ASSERT(stringCache.contains(stringImpl));
> > +    stringCache.remove(stringImpl);
> > +    wrapper.Dispose();
> >  }
> >  
> >  v8::Local<v8::String> v8ExternalString(const String& string)
> 
> A "to" prefix is usually used on conversion functions like this.  (Or perhaps
> you want findCachedV8String?)

David, that's a part of v8 bindings API.  May we postpone it for another CL (if to touch it at all)?

> 
> >  {
> 
> Assert that this is only called on the main thread (since HashMap isn't
> threadsafe). (As long as, that's true.)

See above.

> 
> > +
> > +    StringImpl* stringImpl = string.impl();
> > +    v8::String* s = stringCache.get(stringImpl);
> > +    if (s) return v8::Local<v8::String>(s);
> 
> Avoid abbreviations:
> s/s/cachedV8String/

Done.

Replying to second batch of comments in the corresponding place.

[anton muhin]

(In reply to comment #4)
> > I don't know if it's safe to use v8 string from different threads.
> 
> I don't know about v8 strings, but StringImpl is certainly not safe for use on
> multiple threads.

Thanks a lot for explanations.  Looks like I have no option but to have per thread caches (see the latest patch).  Does it look correct?

> 
> >  If it's the case, I'd only protect the cache with a mutex. 
> 
> That should work for the HashMap (but not the StringImpl in it).

I see, tnx.

> > If it's not, it looks like we need thread locals caches.
> 
> You may want to add to WebCore/platform/ThreadGlobalData.h

I need your advice here.  There is another place in v8 bindings which has thread local storage---V8DOMMap.  However, they don't keep this stuff into ThreadGlobalData either (that's why I decided to keep it 'local' as well).  Overall, I don't see any V8 specific parts in ThreadGlobablData.  Do you think it's an oversight and should be fixed?

> 
> 
> fyi, I've not had time to look into it, but the function v8ValueToWebCoreString
> in V8Binding.cpp has similar concerns due to its use of AtomicString which may
> not be used across multiple threads.  (I don't know if this can be used from
> workers but if it can be, then it should be fixed as well.)

Thank you very much for spotting this.  Christian is working on this part and has a CL under review, maybe it'd be more convenient if he fixes it as well.

Thank you very much for review!

[David Levin]

Comment on attachment 33619 [details]
Next iteration

"
> +#include "StringHash.h"
> +#include "Threading.h"
> +#include "ThreadSpecific.h"

Case sensitive sorting, so it should be:

#include "ThreadSpecific.h"
#include "Threading.h"


> +// Returns thread-specific version of string cache
> +static StringCache& getStringCache()
> +{
> +    if (WTF::isMainThread())
> +    {
> +        DEFINE_STATIC_LOCAL(StringCache, mainThreadStringCache, ());
> +        return mainThreadStringCache;
> +    }
> +
> +    DEFINE_STATIC_LOCAL(WTF::ThreadSpecific<StringCache>, threadStringCache, ());


This looks like it has a race condition. (specifically the allocation of ThreadSpecific<StringCache>.)
Please add a comment about why it isn't or consider switching to using AtomicallyInitializedStatic (from
wtf/Threading.h).

> +static void cachedStringCallback(v8::Persistent<v8::Value> wrapper, void* parameter)
> +{
> +    StringImpl* stringImpl = static_cast<StringImpl*>(parameter);
> +    ASSERT(getStringCache().contains(stringImpl));
> +    getStringCache().remove(stringImpl);
> +    wrapper.Dispose();
> +    stringImpl->deref();

It is my understanding that v8 garbage collection may be done on any thread.  If that is the case, then the code would be broken because getStringCache() returns a thread specific value.

(Even if you got the right string cache, you'd run into problems with StringCache and StringImpl::Deref not being threadsafe.)

[David Levin]

> There is another place in v8 bindings which has
> thread local storage---V8DOMMap.  Do you think
> it's an oversight and should be fixed?

Yes, if it can be used on multiple threads.  I happened to just come across this in a refactoring code review from abarth and flagged it which resulted in https://bugs.webkit.org/show_bug.cgi?id=27759


> Overall, I don't see any V8 specific parts in ThreadGlobablData.  

This class was created afaik to reduce the number of thread local storage items, so I think it would be appropriate to add there and put the appropriate ifdef's for v8.  (Please look at the svn history of that file to double check its history to see if it seems appropriate.)

[anton muhin]

Created attachment 33718 [details]
Next iteration

As was discussed by IM with David, for now I just put caching under a flag.  This flag would be turned on for renderer process in Chromium (but it default to false).

[David Levin]

Comment on attachment 33718 [details]
Next iteration

> Index: third_party/WebKit/WebCore/ChangeLog
> [phone]  Anton Muhin  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Cache v8 strings when converting from WebCore::String to v8 string.
> +        https://bugs.webkit.org/show_bug.cgi?id=27655
> +
> +        * bindings/v8/V8Binding.cpp:
> +        (WebCore::v8String):
> +        (WebCore::cachedStringCallback):
> +        (WebCore::v8ExternalString):

This needs to be updated.  Also per function comments are encourage for non-trivial changes to explain what changes you did to the functions.


> Index: third_party/WebKit/WebCore/bindings/v8/V8Binding.cpp

> +static bool stringImplCacheEnabled = false;
> +
> +void enableStringImplCache()
> +{
> +    stringImplCacheEnabled = true;
> +}

We discussed a compile time flag or runtime.  After seeing this, I much prefer the compile time define because 
1. I think there are several places in the v8 bindings that will need this.
2. I don't think this is a runtime decision, and making it compile time will have the benefits of making the separate code pieces easier to see, reduce complexity of thinking about what happens if the flag is set after some class are done, get rid of an unnecessary branch in your code.

Perhaps:
  V8_SINGLE_THREADED
and only chrome builds define this.


> +    v8::Persistent<v8::String> wrapper = v8::Persistent<v8::String>::New(newString);
> +    if (wrapper.IsEmpty())
> +        return newString;

Does "wrapper.Dispose();" need to be called before the return? (I'm not totally familiar with persistent handles in v8.)


> Index: third_party/WebKit/WebCore/bindings/v8/V8Binding.h
> +    // Enables caching v8 wrappers created for WebCore::StringImpl.  Currently this cache requires
> +    // all the calls (both to convert WebCore::String to v8::String and to GC the handle)
> +    // to be peformed on the main thread.

typo: peformed
(just in case this comment is kept)

[anton muhin]

Created attachment 33769 [details]
Next iteration

[anton muhin]

(In reply to comment #11)
> (From update of attachment 33718 [details])
> > Index: third_party/WebKit/WebCore/ChangeLog
> > [phone]  Anton Muhin  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Cache v8 strings when converting from WebCore::String to v8 string.
> > +        https://bugs.webkit.org/show_bug.cgi?id=27655
> > +
> > +        * bindings/v8/V8Binding.cpp:
> > +        (WebCore::v8String):
> > +        (WebCore::cachedStringCallback):
> > +        (WebCore::v8ExternalString):
> 
> This needs to be updated.  Also per function comments are encourage for
> non-trivial changes to explain what changes you did to the functions.

Hopefully done.  May you have look to see if they are sane.

> 
> 
> > Index: third_party/WebKit/WebCore/bindings/v8/V8Binding.cpp
> 
> > +static bool stringImplCacheEnabled = false;
> > +
> > +void enableStringImplCache()
> > +{
> > +    stringImplCacheEnabled = true;
> > +}
> 
> We discussed a compile time flag or runtime.  After seeing this, I much prefer
> the compile time define because 
> 1. I think there are several places in the v8 bindings that will need this.
> 2. I don't think this is a runtime decision, and making it compile time will
> have the benefits of making the separate code pieces easier to see, reduce
> complexity of thinking about what happens if the flag is set after some class
> are done, get rid of an unnecessary branch in your code.
> 
> Perhaps:
>   V8_SINGLE_THREADED
> and only chrome builds define this.

I'm lacking knowledge of our build process, but are you sure we have to separate binaries for worker and render?  I was under impression that the binary is the same and it flags which turn it into a worker or renderer process.

> 
> 
> > +    v8::Persistent<v8::String> wrapper = v8::Persistent<v8::String>::New(newString);
> > +    if (wrapper.IsEmpty())
> > +        return newString;
> 
> Does "wrapper.Dispose();" need to be called before the return? (I'm not totally
> familiar with persistent handles in v8.)
> \

AFAIK no.

> 
> > Index: third_party/WebKit/WebCore/bindings/v8/V8Binding.h
> > +    // Enables caching v8 wrappers created for WebCore::StringImpl.  Currently this cache requires
> > +    // all the calls (both to convert WebCore::String to v8::String and to GC the handle)
> > +    // to be peformed on the main thread.
> 
> typo: peformed
> (just in case this comment is kept)

Tnx, fixed.

[David Levin]

Comment on attachment 33769 [details]
Next iteration

Last thing to fix....(the paths for the files).

> Index: third_party/WebKit/WebCore/ChangeLog
You seem to be creating these patches from outside of your WebKit enlistment.

These lines should start with WebCore (or else committing them is going to be annoying).

If you create the patch when you are in the third_party/WebKit directory this should all work. Alternately, you could just carefully edit the patch by hand.

> ===================================================================
> --- third_party/WebKit/WebCore/ChangeLog	(revision 46575)
> +++ third_party/WebKit/WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,24 @@
> [phone]  Anton Muhin  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Cache v8 strings when converting from WebCore::String to v8 string.
> +        https://bugs.webkit.org/show_bug.cgi?id=27655
> +
> +        * bindings/v8/V8Binding.cpp:
> +        (WebCore::v8String): now just immediately calls v8ExternalString
> +        (WebCore::enableStringImplCache): enables caching of conversions from WebCore::StringImpl to
> +        v8::String
> +        (WebCore::makeExternalString): utilty function to create external v8::String out of
> +        WebCore::Strin
typo: Strin

I can fix this on landing, but if you upload a new patch, it would be nice to fix it.


> I'm lacking knowledge of our build process, but are you sure we have to
> separate binaries for worker and render?  I was under impression that the
> binary is the same and it flags which turn it into a worker or renderer
> process.

Yes. Note: Workers in chromium only run one thread, but we can leave this as a runtime flag as it all appears to work.

The api feels a bit narrow in scope "enableStringImplCache" vs "setSingleThreadedMode", but this is fine. It can be broadened if there is ever any other code that needs this info.

[anton muhin]

Created attachment 33776 [details]
Next iteration

[anton muhin]

David, thanks a lot!

(In reply to comment #14)
> (From update of attachment 33769 [details])
> Last thing to fix....(the paths for the files).
> 
> > Index: third_party/WebKit/WebCore/ChangeLog
> You seem to be creating these patches from outside of your WebKit enlistment.
> 
> These lines should start with WebCore (or else committing them is going to be
> annoying).
> 
> If you create the patch when you are in the third_party/WebKit directory this
> should all work. Alternately, you could just carefully edit the patch by hand.

svn-create-patch doesn't quite work for me (or I just misuse it).  Tried to manually fix it, sorry if missed anything.

> 
> > ===================================================================
> > --- third_party/WebKit/WebCore/ChangeLog	(revision 46575)
> > +++ third_party/WebKit/WebCore/ChangeLog	(working copy)
> > @@ -1,3 +1,24 @@
> > [phone]  Anton Muhin  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Cache v8 strings when converting from WebCore::String to v8 string.
> > +        https://bugs.webkit.org/show_bug.cgi?id=27655
> > +
> > +        * bindings/v8/V8Binding.cpp:
> > +        (WebCore::v8String): now just immediately calls v8ExternalString
> > +        (WebCore::enableStringImplCache): enables caching of conversions from WebCore::StringImpl to
> > +        v8::String
> > +        (WebCore::makeExternalString): utilty function to create external v8::String out of
> > +        WebCore::Strin
> typo: Strin
> 
> I can fix this on landing, but if you upload a new patch, it would be nice to
> fix it.

Fixed.

> 
> 
> > I'm lacking knowledge of our build process, but are you sure we have to
> > separate binaries for worker and render?  I was under impression that the
> > binary is the same and it flags which turn it into a worker or renderer
> > process.
> 
> Yes. Note: Workers in chromium only run one thread, but we can leave this as a
> runtime flag as it all appears to work.
> 
> The api feels a bit narrow in scope "enableStringImplCache" vs
> "setSingleThreadedMode", but this is fine. It can be broadened if there is ever
> any other code that needs this info.

So as was discussed over IM, let's leave it for now.

[David Levin]

Committed as http://trac.webkit.org/changeset/46580