[webkit-reviews] review denied: [Bug 27762] [v8] Cache atomic strings in externalized v8 strings : [Attachment 33865] Next iteration

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied David Levin <levin at chromium.org>'s
request for review:
Bug 27762: [v8] Cache atomic strings in externalized v8 strings
https://bugs.webkit.org/show_bug.cgi?id=27762

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

------- Additional Comments from David Levin <levin at chromium.org>
Just a few more thigns.

> Index: WebCore/ChangeLog
> +2009-07-31  Christian Plesner Hansen  <christian.plesner.hansen at gmail.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   [V8] Cache atomic strings in externalized v8 strings
> +	   https://bugs.webkit.org/show_bug.cgi?id=27762
> +
> +	   * bindings/scripts/CodeGeneratorV8.pm:
> +	   * bindings/v8/V8Binding.cpp:
> +	   (WebCore::WebCoreStringResource::WebCoreStringResource):
> +	   (WebCore::WebCoreStringResource::~WebCoreStringResource):
> +	   (WebCore::WebCoreStringResource::data):
> +	   (WebCore::WebCoreStringResource::length):
> +	   (WebCore::WebCoreStringResource::webcoreString):
> +	   (WebCore::WebCoreStringResource::atomicString):
> +	   (WebCore::WebCoreStringResource::forString):
> +	   (WebCore::v8StringToWebCoreString):
> +	   (WebCore::v8StringToAtomicWebCoreString):
> +	   (WebCore::v8ValueToWebCoreString):
> +	   (WebCore::v8ValueToAtomicWebCoreString):

I think this is ok for this change, but in general (brief) per function
comments are encouraged to explain what happened in the function.  See other
entries in the change log.

> Index: WebCore/bindings/v8/V8Binding.cpp

> @@ -49,33 +49,66 @@ namespace WebCore {
>  class WebCoreStringResource : public v8::String::ExternalStringResource {
>  public:
>      explicit WebCoreStringResource(const String& string)
> -	   : m_impl(string.impl())
> +	   : m_plainString(string)
>      {
Nice to add assert here:
	ASSERT(WTF::isMainThread());

>	   v8::V8::AdjustAmountOfExternalAllocatedMemory(2 * length());
>      }
>  
> +    explicit WebCoreStringResource(const AtomicString& string)
> +	   : m_plainString(string)
> +	   , m_atomicString(string)
> +    {
> +	   ASSERT(WTF::isMainThread());
> +	   v8::V8::AdjustAmountOfExternalAllocatedMemory(2 * length());
> +    }
> +
>      virtual ~WebCoreStringResource()
>      {
> -	   v8::V8::AdjustAmountOfExternalAllocatedMemory(-2 * length());

Nice to add assert here:
	ASSERT(WTF::isMainThread());

(Really all we care about is that everything is on the same thread and for now
this will do.)


> +	   int reducedExternalMemory = -2 * length();
> +	   if (!m_plainString.impl()->inTable())
> +	       reducedExternalMemory *= 2;
> +	  
v8::V8::AdjustAmountOfExternalAllocatedMemory(reducedExternalMemory);
>      }



> +    AtomicString atomicString()
> +    {
> +	   ASSERT(WTF::isMainThread());
> +	   if (m_atomicString.isNull()) {
> +	       m_atomicString = AtomicString(m_plainString);
> +	       if (m_plainString.impl()->inTable())

This should be
   if (!m_plainString.impl()->inTable())

> +		   v8::V8::AdjustAmountOfExternalAllocatedMemory(2 * length());

> +	   }
> +	   return m_atomicString;
> +    }
> +
> +    static WebCoreStringResource *forString(v8::Handle<v8::String> v8String)


Move the "*"
   static WebCoreStringResource*

Also the name "forString" doesn't read that well to me. How about
toStringResource?  (toString may have worked but there are so many *String* in
here, that I thought it would be nice to distinguish.)


> +AtomicString v8StringToAtomicWebCoreString(v8::Handle<v8::String> v8String)
> +{
> +    WebCoreStringResource* stringResource =
WebCoreStringResource::forString(v8String);
> +    if (!stringResource) {
> +	   // If this string hasn't been externalized we force it now.

Nice to add a comma after the if clause.
   If this string hasn't been externalized, we force it now.



And as discussed, this solution removed the duplicate code but increased memory
usage unnecessarily.