Created attachment 132034 [details] Performance test We can optimize the performance of innerHTML and outerHTML more. The performance test is attached.
Created attachment 132035 [details] Patch
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.
Very impressive.
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.
(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.
Patch looks great. The new code looks considerably simpler and more maintainable too!
Created attachment 132161 [details] Patch
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.
+rniwa for fun with MarkupAccumulator.cpp.
(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*&)'
(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.
> const char Doctype[] = "<!DOCTYPE "; > WTF_ARRAY_LENGTH(DocType) should work. ^^^ WTF_ARRAY_LENGTH(DocType) - 1 :)
Created attachment 132172 [details] Patch
(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'.
Yay, you're doing my work (making them faster was on my OKRs).
Comment on attachment 132172 [details] Patch The patch passed all ews-bots. If the change looks ok, would anyone r+ it?
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.
(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 on attachment 132172 [details] Patch The patch looks great to me as well
The patch looks great to me as well
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.
(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.
(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").
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 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.
Created attachment 132209 [details] patch for landing
Comment on attachment 132209 [details] patch for landing Clearing flags on attachment: 132209 Committed r110992: <http://trac.webkit.org/changeset/110992>