Bug 81214 - [Performance] Optimize innerHTML and outerHTML
Summary: [Performance] Optimize innerHTML and outerHTML
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on: 81288
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-15 06:30 PDT by Kentaro Hara
Modified: 2012-03-16 05:35 PDT (History)
9 users (show)

See Also:


Attachments
Performance test (115.71 KB, text/html)
2012-03-15 06:30 PDT, Kentaro Hara
no flags Details
Patch (23.28 KB, patch)
2012-03-15 06:32 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (13.38 KB, patch)
2012-03-15 17:33 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (13.96 KB, patch)
2012-03-15 18:32 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
patch for landing (13.89 KB, patch)
2012-03-15 23:23 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-03-15 06:30:14 PDT
Created attachment 132034 [details]
Performance test

We can optimize the performance of innerHTML and outerHTML more. The performance test is attached.
Comment 1 Kentaro Hara 2012-03-15 06:32:54 PDT
Created attachment 132035 [details]
Patch
Comment 2 Kentaro Hara 2012-03-15 06:34:25 PDT
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.
Comment 3 Adam Barth 2012-03-15 11:05:42 PDT
Very impressive.
Comment 4 Alexey Proskuryakov 2012-03-15 11:05:54 PDT
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.
Comment 5 Kentaro Hara 2012-03-15 15:26:42 PDT
(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.
Comment 6 Ojan Vafai 2012-03-15 15:29:09 PDT
Patch looks great. The new code looks considerably simpler and more maintainable too!
Comment 7 Kentaro Hara 2012-03-15 17:33:40 PDT
Created attachment 132161 [details]
Patch
Comment 8 Tony Chang 2012-03-15 17:56:02 PDT
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.
Comment 9 Adam Barth 2012-03-15 18:05:15 PDT
+rniwa for fun with MarkupAccumulator.cpp.
Comment 10 Kentaro Hara 2012-03-15 18:18:37 PDT
(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*&)'
Comment 11 Tony Chang 2012-03-15 18:22:33 PDT
(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.
Comment 12 Adam Barth 2012-03-15 18:24:53 PDT
> const char Doctype[] = "<!DOCTYPE ";
> WTF_ARRAY_LENGTH(DocType) should work.

^^^ WTF_ARRAY_LENGTH(DocType) - 1 :)
Comment 13 Kentaro Hara 2012-03-15 18:32:11 PDT
Created attachment 132172 [details]
Patch
Comment 14 Kentaro Hara 2012-03-15 18:33:27 PDT
(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'.
Comment 15 Ryosuke Niwa 2012-03-15 21:11:06 PDT
Yay, you're doing my work (making them faster was on my OKRs).
Comment 16 Kentaro Hara 2012-03-15 21:19:31 PDT
Comment on attachment 132172 [details]
Patch

The patch passed all ews-bots. If the change looks ok, would anyone r+ it?
Comment 17 Adam Barth 2012-03-15 21:39:52 PDT
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.
Comment 18 Kentaro Hara 2012-03-15 21:41:41 PDT
(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.
Comment 19 Ryosuke Niwa 2012-03-15 21:44:30 PDT
Comment on attachment 132172 [details]
Patch

The patch looks great to me as well
Comment 20 Ryosuke Niwa 2012-03-15 21:44:39 PDT
The patch looks great to me as well
Comment 21 Alexey Proskuryakov 2012-03-15 21:53:48 PDT
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.
Comment 22 Kentaro Hara 2012-03-15 21:58:39 PDT
(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.
Comment 23 Ryosuke Niwa 2012-03-15 22:26:20 PDT
(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").
Comment 24 Alexey Proskuryakov 2012-03-15 22:55:35 PDT
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.
Comment 25 Kentaro Hara 2012-03-15 23:17:59 PDT
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.
Comment 26 Kentaro Hara 2012-03-15 23:23:11 PDT
Created attachment 132209 [details]
patch for landing
Comment 27 WebKit Review Bot 2012-03-16 05:32:14 PDT
Comment on attachment 132209 [details]
patch for landing

Clearing flags on attachment: 132209

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