[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 #13 from anton muhin <antonm at chromium.org>  2009-07-30 05:13:44 PDT ---
(In reply to comment #11)
> (From update of attachment 33718 [details])
> > Index: third_party/WebKit/WebCore/ChangeLog
> > +2009-07-24  Anton Muhin  <antonm at chromium.org>
> > +
> > +        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.

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