[Webkit-unassigned] [Bug 27655] [v8] cache v8 strings when converting from webcore string to v8 string

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27655





--- Comment #3 from anton muhin <antonm at chromium.org>  2009-07-25 13:51:08 PDT ---
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/

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.