Bug 81214 – [Performance] Optimize innerHTML and outerHTML

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Kentaro Hara]

Created attachment 132034 [details]
Performance test

We can optimize the performance of innerHTML and outerHTML more. The performance test is attached.

[Kentaro Hara]

Created attachment 132035 [details]
Patch

[Kentaro Hara]

Comment on attachment 132035 [details]
Patch

The uploaded patch makes innerHTML and outerHTML 2.4 times faster. Since the patch is doing too many things, I will land it step by step. Comments are appreciated.

[Adam Barth]

Very impressive.

[Alexey Proskuryakov]

Does this speed up any widely recognized benchmark? If a change only improves a synthetic benchmark made just for it, then it may not be worth the code churn.

[Kentaro Hara]

(In reply to comment #4)
> Does this speed up any widely recognized benchmark? If a change only improves a synthetic benchmark made just for it, then it may not be worth the code churn.

I couldn't find existing benchmarks that test innerHTML. But the attached performance benchmark tests body.innerHTML for 3000 lines of HTML, which is copied from Dromaeo/dom-attr.html. Thus I think the performance impact will be practical.

[Ojan Vafai]

Patch looks great. The new code looks considerably simpler and more maintainable too!

[Kentaro Hara]

Created attachment 132161 [details]
Patch

[Tony Chang]

Comment on attachment 132161 [details]
Patch

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

> Source/WebCore/editing/MarkupAccumulator.cpp:302
> -    result.append("<!DOCTYPE ");
> +    result.append("<!DOCTYPE ", 10);

I would pull these out into const char* variables and use WTF_ARRAY_LENGTH instead of hard coding the lengths.

[Adam Barth]

+rniwa for fun with MarkupAccumulator.cpp.

[Kentaro Hara]

(In reply to comment #8)
> > Source/WebCore/editing/MarkupAccumulator.cpp:302
> > -    result.append("<!DOCTYPE ");
> > +    result.append("<!DOCTYPE ", 10);
> 
> I would pull these out into const char* variables and use WTF_ARRAY_LENGTH instead of hard coding the lengths.

tony: It seems we cannot call WTF_ARRAY_LENGTH for char* array... (without a hack in StdLibExtras.h)

    third_party/WebKit/Source/WebCore/editing/MarkupAccumulator.cpp: In static member function 'static void  WebCore::MarkupAccumulator::appendComment(WTF::StringBuilder&, const WTF::String&)':
    third_party/WebKit/Source/WebCore/editing/MarkupAccumulator.cpp:293: error: no matching function for call to 'ArrayLengthHelperFunction(const char*&)'
    third_party/WebKit/Source/WebCore/editing/MarkupAccumulator.cpp:296: error: no matching function for call to 'ArrayLengthHelperFunction(const char*&)'

[Tony Chang]

(In reply to comment #10)
> (In reply to comment #8)
> > > Source/WebCore/editing/MarkupAccumulator.cpp:302
> > > -    result.append("<!DOCTYPE ");
> > > +    result.append("<!DOCTYPE ", 10);
> > 
> > I would pull these out into const char* variables and use WTF_ARRAY_LENGTH instead of hard coding the lengths.
> 
> tony: It seems we cannot call WTF_ARRAY_LENGTH for char* array... (without a hack in StdLibExtras.h)

Sorry, you can do:

const char Doctype[] = "<!DOCTYPE ";
WTF_ARRAY_LENGTH(DocType) should work.

[Adam Barth]

> const char Doctype[] = "<!DOCTYPE ";
> WTF_ARRAY_LENGTH(DocType) should work.

^^^ WTF_ARRAY_LENGTH(DocType) - 1 :)

[Kentaro Hara]

Created attachment 132172 [details]
Patch

[Kentaro Hara]

(In reply to comment #12)
> > const char Doctype[] = "<!DOCTYPE ";
> > WTF_ARRAY_LENGTH(DocType) should work.
> 
> ^^^ WTF_ARRAY_LENGTH(DocType) - 1 :)

Fixed. Thanks. I confirmed no performance difference between 'WTF_ARRAY_LENGTH(DocType) - 1' and hard-coding '10'.

[Ryosuke Niwa]

Yay, you're doing my work (making them faster was on my OKRs).

[Kentaro Hara]

Comment on attachment 132172 [details]
Patch

The patch passed all ews-bots. If the change looks ok, would anyone r+ it?

[Adam Barth]

Comment on attachment 132172 [details]
Patch

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

The old code looks kind of silly after this patch.  :)

> Source/WebCore/editing/MarkupAccumulator.cpp:156
>  void MarkupAccumulator::concatenateMarkup(StringBuilder& result)

This function appears unused.  Perhaps we can remove it?

> Source/WebCore/editing/MarkupAccumulator.h:81
>      static size_t totalLength(const Vector<String>&);

Does this function have any more callers?  Perhaps we can remove it.

[Kentaro Hara]

(In reply to comment #17)
> > Source/WebCore/editing/MarkupAccumulator.cpp:156
> >  void MarkupAccumulator::concatenateMarkup(StringBuilder& result)
> 
> This function appears unused.  Perhaps we can remove it?
> 
> > Source/WebCore/editing/MarkupAccumulator.h:81
> >      static size_t totalLength(const Vector<String>&);
> 
> Does this function have any more callers?  Perhaps we can remove it.

We cannot remove them because they are still used by markup.cpp. We should improve markup.cpp later.

[Ryosuke Niwa]

Comment on attachment 132172 [details]
Patch

The patch looks great to me as well

[Ryosuke Niwa]

The patch looks great to me as well

[Alexey Proskuryakov]

Comment on attachment 132172 [details]
Patch

Why is this using WTF_ARRAY_LENGTH and not just sizeof? Size of char is guaranteed to be 1, we're not winning anything with fancy macros.

[Kentaro Hara]

(In reply to comment #21)
> (From update of attachment 132172 [details])
> Why is this using WTF_ARRAY_LENGTH and not just sizeof? Size of char is guaranteed to be 1, we're not winning anything with fancy macros.

I guess either is OK, WTF_ARRAY_LENGTH is just an alias of sizeof in this case. As far as I see the code around, it seems we are likely to use WTF_ARRAY_LENGTH (instead of sizeof) since we do not need to care types.

[Ryosuke Niwa]

(In reply to comment #21)
> (From update of attachment 132172 [details])
> Why is this using WTF_ARRAY_LENGTH and not just sizeof? Size of char is guaranteed to be 1, we're not winning anything with fancy macros.

I think it's probably better to use WTF_ARRAY_LENGTH so that we don't have to fix the code when we change the string type in the future for example (e.g. L"text").

[Alexey Proskuryakov]

I'm quite confident that this is a wrong way to look at this. Using more complicated and uglier code today just in case that's needed for a future refactoring we don't even expect to ever happen is solving the wrong problem.

[Kentaro Hara]

Comment on attachment 132172 [details]
Patch

(In reply to comment #24)
> I'm quite confident that this is a wrong way to look at this. Using more complicated and uglier code today just in case that's needed for a future refactoring we don't even expect to ever happen is solving the wrong problem.

OK, then I'll use sizeof (I do not have a strong opinion for this). I think both opinions make sense. You are right, and rniwa and tony are also right.

[Kentaro Hara]

Created attachment 132209 [details]
patch for landing

[WebKit Review Bot]

Comment on attachment 132209 [details]
patch for landing

Clearing flags on attachment: 132209

Committed r110992: <http://trac.webkit.org/changeset/110992>