[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


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #33718|review?                     |review-
               Flag|                            |




--- Comment #11 from David Levin <levin at chromium.org>  2009-07-29 10:25:56 PDT ---
(From update of attachment 33718)
> 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.


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

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