[Webkit-unassigned] [Bug 27762] [v8] Cache atomic strings in externalized v8 strings

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27762


David Levin <levin at chromium.org> changed:

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




--- Comment #28 from David Levin <levin at chromium.org>  2009-07-31 09:01:29 PDT ---
(From update of attachment 33865)
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.

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