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




--- Comment #25 from David Levin <levin at chromium.org>  2009-07-30 20:58:12 PDT ---
(From update of attachment 33721)
Just a few things to address and this can be done.

Please get the patch to be based from WebKit or else it will be harder to
commit this for you (i.e. in this case, all path for files below should be
prefixed with WebCore/).

> Index: ChangeLog
> ===================================================================
> --- ChangeLog	(revision 46477)
> +++ ChangeLog	(working copy)
> @@ -1,3 +1,21 @@
> +2009-07-28  Christian Plesner Hansen  <christian.plesner.hansen at gmail.com>
> +
> +    Reviewed by NOBODY (OOPS!).
> +
Add title and bug link like this:

[v8] Cache atomic strings in externalized v8 strings
https://bugs.webkit.org/show_bug.cgi?id=27762


> +    Cache atomic strings in externalized strings the first time
> +    the string is converted.  Added V8Custom IDL annotation for
> +    hinting that converting a string directly to an atomic string is a
> +    good idea.

Get rid of TABs in the changelog.



> Index: bindings/v8/V8Binding.cpp
> +    explicit WebCoreStringResource(const AtomicString& string)
> +        : m_impl(string.impl()), m_atomicImpl(string.impl())
Please format like this:
    explicit WebCoreStringResource(const AtomicString& string)
        : m_impl(string.impl())
        , m_atomicImpl(string.impl())




> +    AtomicString atomicString()
> +    {
> +        ASSERT(WTF::isMainThread());
> +        if (m_atomicImpl.isNull())
> +            m_atomicImpl = AtomicString(m_impl);

It seems that you should adjust the external memory here:
        if (m_atomicImpl.isNull()) {
            m_atomicImpl = AtomicString(m_impl);
            if (!m_impl.inTable())
                v8::V8::AdjustAmountOfExternalAllocatedMemory(2 * length());
        }

And change the destructor like this:

    virtual ~WebCoreStringResource()
    {
        ASSERT(WTF::isMainThread());
        int reducedExternalMemory = -2 * length();
        if (!m_impl.inTable())
            reducedExternalMemory *= 2;
        v8::V8::AdjustAmountOfExternalAllocatedMemory(reducedExternalMemory);
    }

>  private:
>      // A shallow copy of the string. Keeps the string buffer alive until the V8 engine garbage collects it.
>      String m_impl;
> +    // If this string is atomic or has been made atomic earlier the
> +    // atomic string is held here.  In the case where the string starts

Please only put one space after periods.

> +    // off non-atomic and becomes atomic later it is necessary to keep
> +    // the original string alive because v8 may keep derived pointers
> +    // into that string.
> +    AtomicString m_atomicImpl;

Why is it m_atomicImpl instead of just m_atomicString?


> @@ -130,13 +151,34 @@ String v8ValueToWebCoreString(v8::Handle
>  
>  AtomicString v8StringToAtomicWebCoreString(v8::Handle<v8::String> v8String)
>  {
> +    WebCoreStringResource* stringResource = static_cast<WebCoreStringResource*>(v8String->GetExternalStringResource());
> +    if (stringResource)
> +        return stringResource->atomicString();
> +
> +    int length = v8String->Length();
> +    if (!length) {
> +        // Avoid trying to morph empty strings, as they do not have enough room to contain the external reference.
> +        return StringImpl::empty();
> +    }
> +
> +    UChar* buffer;
> +    String plainResult = String::createUninitialized(length, buffer);
> +    v8String->Write(reinterpret_cast<uint16_t*>(buffer), 0, length);
> +    AtomicString result(plainResult);
> +
> +    WebCoreStringResource* resource = new WebCoreStringResource(result);
> +    if (!v8String->MakeExternal(resource)) {
> +        // In case of a failure delete the external resource as it was not used.
> +        delete resource;
> +    }
> +    return result;

This duplicates a fair amount of code from v8StringToWebCoreString. Can you
factor the code to avoid this?

btw, if you create a new function (or add to an existing function) and want a
"bool" parameter, please make it an enum instead.

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