[webkit-reviews] review denied: [Bug 27655] [v8] cache v8 strings when converting from webcore string to v8 string : [Attachment 33619] Next iteration

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied anton muhin <antonm at chromium.org>'s
request for review:
Bug 27655: [v8] cache v8 strings when converting from webcore string to v8
string
https://bugs.webkit.org/show_bug.cgi?id=27655

Attachment 33619: Next iteration
https://bugs.webkit.org/attachment.cgi?id=33619&action=review

------- Additional Comments from David Levin <levin at chromium.org>
"
> +#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.)