[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 #33453|review?                     |review-
               Flag|                            |




--- Comment #2 from David Levin <levin at chromium.org>  2009-07-24 09:49:51 PDT ---
(From update of attachment 33453)
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.