[Webkit-unassigned] [Bug 31415] [v8] do not copy data twice when converting short v8 string into WebCore::String

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from anton muhin <antonm at chromium.org>  2009-11-13 05:08:50 PST ---
(In reply to comment #5)
> (From update of attachment 43080 [details])
> > Index: WebCore/ChangeLog
> 
> > +        Do not use WebCore::String::String(const UChar*, int length) to convert
> > +        short v8 strings.
> > +
> > +        Plus added string traits.
> 
> It is good to add the reason for the change to the ChangeLog as well as what
> was done.
> Ideally, perf optimizations would come with some measurement to show the actual
> win.

It has no performance impact or completely minimal.  The idea was not to speed
up some things, but to make it clear that this optimization is actually wrong
for this particular case.

> 
> > +    static String convertFromV8(v8::Handle<v8::String> v8String, int length) {
> > +        ASSERT(v8String->Length() == length);
> > +        // NOTE: as of now, String(const UChar*, int) performs String::createUninitialized
> > +        // anyway, so no need to optimize like we do for AtomicString below.
> > +        UChar* buffer;
> > +        String result = String::createUninitialized(length, buffer);
> > +        v8String->Write(reinterpret_cast<uint16_t*>(buffer), 0, length);
> > +        return result;
> 
> It is unfortunate that this introduces almost a duplicate of the code... Could
> you consider having the StringTraits to have some simple bool flag, like
> "useInlineBuffer" and have a single copy of the code, something like this:
> if (StringTraits<StringType>::useInlineBuffer() && length <= inlineBufferSize)
> ... use inline buffer ...
> } else {
> ... use createUninitialized ...
> }
> or some other way to keep the code in one place?

Sure we could.  The problem: there are only three lines of the code (including
UChar* buffer declaration), so I am not sure that in this particular case it's
worth the effort.  But if you still think it's a good idea, I'd definitely do
that.

And thanks a lot for comments.

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