Bug 79125 – CachedScriptSourceProvider::getRange can unnecessarily make a full copy of the source

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebCore JavaScript

[Yong Li]

JSC::UString getRange(int start, int end) should share the existing StringImpl when it is asked to return the full range.

patch is forthcoming

[Yong Li]

Created attachment 127998 [details]
the patch

[Philippe Normand]

Comment on attachment 127998 [details]
the patch

Attachment 127998 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11562167

[Early Warning System Bot]

Comment on attachment 127998 [details]
the patch

Attachment 127998 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11562168

[Yong Li]

Created attachment 128001 [details]
Try again

[Yong Li]

Comment on attachment 128001 [details]
Try again

Actually I can make it better

[Yong Li]

Created attachment 128026 [details]
another try

[Yong Li]

Created attachment 128031 [details]
updated

[Yong Li]

Created attachment 128219 [details]
More

Prevent CachedScript from resetting m_script when its string buffer is still referenced by others because doing so won't save any memory but cause extra memory usage if scritp() is called on the CachedScript afterwards.

[Yong Li]

Comment on attachment 128219 [details]
More

The change in CachedScript may be unnecessary and also uncomplete.

[Yong Li]

Comment on attachment 128031 [details]
updated

Go back to this simple change

[Yong Li]

Created attachment 140039 [details]
new patch

[Eric Seidel (no email)]

Comment on attachment 140039 [details]
new patch

Don't we have a String::substring() or StringImpl::substring() call which would do this optimization for us?

[Build Bot]

Comment on attachment 140039 [details]
new patch

Attachment 140039 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12606210

[Yong Li]

(In reply to comment #12)
> (From update of attachment 140039 [details])
> Don't we have a String::substring() or StringImpl::substring() call which would do this optimization for us?

Yes we do. My previous patch uses substringSharingImpl(). But I feel it is a little bit confusing because it hints that, no matter where the substring is, it would always share the original buffer. That might or might not be good, because the CachedScript will destroy decoded script. Actually substringSharingImpl() creates new copy if the substring is not the full string. But the name isn't so accurate.

[Darin Adler]

Comment on attachment 140039 [details]
new patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140039&action=review

> Source/WebCore/bindings/js/CachedScriptSourceProvider.h:59
>              ASSERT(start + length <= this->length());
>  
>              String script = m_cachedScript->script();
> +
> +            if (!start && length == script.length())
> +                return stringToUString(script);
> +
>              return JSC::UString(StringImpl::create(script.impl(), start, length));

This whole thing can be done as a one-liner, without all those assertions, using the substring function, which already correctly optimizes the case of a range covering the entire string.

    return stringToUString(m_cachedScript->script().substring(start, end - start));

> Source/WebCore/bindings/js/StringSourceProvider.h:54
>              ASSERT(length >= 0);
>              ASSERT(start + length <= this->length());
>  
> +            if (!start && length == m_source.length())
> +                return stringToUString(m_source);
> +
>              return JSC::UString(StringImpl::create(m_source.impl(), start, length));

Same here:

    return stringToUString(m_source.substring(start, end - start));

[Yong Li]

(In reply to comment #15)
> (From update of attachment 140039 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140039&action=review
> 
> > Source/WebCore/bindings/js/CachedScriptSourceProvider.h:59
> >              ASSERT(start + length <= this->length());
> >  
> >              String script = m_cachedScript->script();
> > +
> > +            if (!start && length == script.length())
> > +                return stringToUString(script);
> > +
> >              return JSC::UString(StringImpl::create(script.impl(), start, length));
> 
> This whole thing can be done as a one-liner, without all those assertions, using the substring function, which already correctly optimizes the case of a range covering the entire string.
> 
>     return stringToUString(m_cachedScript->script().substring(start, end - start));
> 


Hm... Didn't realize substring() can do that since some day.

substring() also does sanity checks. so no need for those asserts now. patch is forthcoming

[Yong Li]

Created attachment 140246 [details]
use substring() now

[Yong Li]

Created attachment 140247 [details]
substring()

[Yong Li]

Comment on attachment 140247 [details]
substring()

Actually the issue has been fixed with the latest code:

http://trac.webkit.org/changeset/111358
Bug 81466

[Yong Li]

*** This bug has been marked as a duplicate of bug 81466 ***