JSC::UString getRange(int start, int end) should share the existing StringImpl when it is asked to return the full range. patch is forthcoming
Created attachment 127998 [details] the patch
Comment on attachment 127998 [details] the patch Attachment 127998 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11562167
Comment on attachment 127998 [details] the patch Attachment 127998 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11562168
Created attachment 128001 [details] Try again
Comment on attachment 128001 [details] Try again Actually I can make it better
Created attachment 128026 [details] another try
Created attachment 128031 [details] updated
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.
Comment on attachment 128219 [details] More The change in CachedScript may be unnecessary and also uncomplete.
Comment on attachment 128031 [details] updated Go back to this simple change
Created attachment 140039 [details] new patch
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?
Comment on attachment 140039 [details] new patch Attachment 140039 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12606210
(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.
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));
(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
Created attachment 140246 [details] use substring() now
Created attachment 140247 [details] substring()
Comment on attachment 140247 [details] substring() Actually the issue has been fixed with the latest code: http://trac.webkit.org/changeset/111358 Bug 81466
*** This bug has been marked as a duplicate of bug 81466 ***