Bug 81192 - [Chromium][Performance] Optimize innerText and outerText in Chromium/Mac
: [Chromium][Performance] Optimize innerText and outerText in Chromium/Mac
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-03-14 23:00 PST by
Modified: 2012-03-18 10:00 PST (History)


Attachments
Performance test (542 bytes, text/html)
2012-03-14 23:00 PST, Kentaro Hara
no flags Details
Patch (6.18 KB, patch)
2012-03-14 23:04 PST, Kentaro Hara
no flags Review Patch | Details | Formatted Diff | Diff
Updated performance test (802 bytes, text/html)
2012-03-15 00:04 PST, Kentaro Hara
no flags Details
Patch (6.88 KB, patch)
2012-03-15 00:09 PST, Kentaro Hara
no flags Review Patch | Details | Formatted Diff | Diff
Patch (3.74 KB, patch)
2012-03-15 07:00 PST, Kentaro Hara
no flags Review Patch | Details | Formatted Diff | Diff
Patch (3.79 KB, patch)
2012-03-15 21:12 PST, Kentaro Hara
no flags Review Patch | Details | Formatted Diff | Diff
Patch (2.63 KB, patch)
2012-03-18 04:48 PST, Kentaro Hara
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-03-14 23:00:49 PST
Created an attachment (id=131989) [details]
Performance test

In Mac, HTMLElement.innerText in Chromium/V8 is 3~ times slower than HTMLElement.innerText in AppleWebKit/JavaScriptCore. We should optimize it.

The results of the attached performance test in my local Mac environemnt are as follows:

- Chromium/V8: 10250.8 ms
- AppleWebKit/JavaScriptCore: 3075.4 ms
------- Comment #1 From 2012-03-14 23:04:24 PST -------
Created an attachment (id=131990) [details]
Patch
------- Comment #2 From 2012-03-15 00:04:26 PST -------
Created an attachment (id=131994) [details]
Updated performance test

The change improves the performance of outerText too. Here are the results in my local Mac:

- AppleWebKit/JavaScriptCore
div.innerText : 2978.4ms
div.outerText : 2944.4ms

- Chromium/V8 without the patch
div.innerText : 10050.8ms
div.outerText : 10072.2ms

- Chromium/V8 with the patch
div.innerText: 2536.4ms
div.outerText: 2714ms
------- Comment #3 From 2012-03-15 00:09:49 PST -------
Created an attachment (id=131995) [details]
Patch
------- Comment #4 From 2012-03-15 04:43:45 PST -------
(From update of attachment 131995 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=131995&action=review

I'll leave reviewing to Chromium people.

> Source/JavaScriptCore/wtf/Platform.h:1195
> +#if PLATFORM(CHROMIUM)
> +#define WTF_TEXT_ITERATOR_BUFFER_INITIAL_CAPACITY (1 << 15)
> +#else
> +#define WTF_TEXT_ITERATOR_BUFFER_INITIAL_CAPACITY (1 << 16)
> +#endif /* PLATFORM(CHROMIUM) */

This is too small for Platform.h. It should be in the function itself.
------- Comment #5 From 2012-03-15 07:00:32 PST -------
Created an attachment (id=132038) [details]
Patch
------- Comment #6 From 2012-03-15 07:14:20 PST -------
(In reply to comment #4)
> > Source/JavaScriptCore/wtf/Platform.h:1195
> > +#if PLATFORM(CHROMIUM)
> > +#define WTF_TEXT_ITERATOR_BUFFER_INITIAL_CAPACITY (1 << 15)
> > +#else
> > +#define WTF_TEXT_ITERATOR_BUFFER_INITIAL_CAPACITY (1 << 16)
> > +#endif /* PLATFORM(CHROMIUM) */
> 
> This is too small for Platform.h. It should be in the function itself.

Fixed. Thanks.
------- Comment #7 From 2012-03-15 10:25:25 PST -------
(From update of attachment 132038 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=132038&action=review

Whoa, this is an awesome patch. For a second there, I was excited that you were speeding up the setters. But even speeding up the getters so much is kick-ass!

> Source/WebCore/editing/TextIterator.cpp:2515
> +#if PLATFORM(CHROMIUM)

Why CHROMIUM? That is way too wide and completely mysterious. Can we narrow down the specific bit that's required?
------- Comment #8 From 2012-03-15 13:33:35 PST -------
(In reply to comment #7)
This issue may depend on the fact that Chromium/Mac doesn't use tcmalloc.  So I think we can narrow it down to Mac.

Great work, haraken.
------- Comment #9 From 2012-03-15 17:39:19 PST -------
(From update of attachment 132038 [details])
Thanks for comments. Let me invalidate r? for now to investigate more. A new patch is coming soon.
------- Comment #10 From 2012-03-15 21:12:51 PST -------
Created an attachment (id=132190) [details]
Patch
------- Comment #11 From 2012-03-15 21:16:53 PST -------
(In reply to comment #7)
> > Source/WebCore/editing/TextIterator.cpp:2515
> > +#if PLATFORM(CHROMIUM)
> 
> Why CHROMIUM? That is way too wide and completely mysterious. Can we narrow down the specific bit that's required?

(In reply to comment #8)
> (In reply to comment #7)
> This issue may depend on the fact that Chromium/Mac doesn't use tcmalloc.  So I think we can narrow it down to Mac.

Fixed it.

Another thing I tried is to remove plainTextToMallocAllocatedBuffer() and replace the logic with StringBuilder. While it cleaned up the code, it degraded performance by 10%.

Consequently, I just changed the initial buffer size if "PLATFORM(CHROMIUM) && PLATFORM(MAC)".
------- Comment #12 From 2012-03-16 19:33:07 PST -------
(From update of attachment 132190 [details])
Clearing flags on attachment: 132190

Committed r111109: <http://trac.webkit.org/changeset/111109>
------- Comment #13 From 2012-03-16 19:33:12 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #14 From 2012-03-17 19:17:08 PST -------
Why was this even made a platform ifdef at all? Is there any platform where 1 << 15 is worse than 1 << 16? If not, we should just keep the value consistent.
------- Comment #15 From 2012-03-17 19:20:35 PST -------
(In reply to comment #14)
> Why was this even made a platform ifdef at all? Is there any platform where 1 << 15 is worse than 1 << 16? If not, we should just keep the value consistent.

Even better.
------- Comment #16 From 2012-03-17 20:34:05 PST -------
(In reply to comment #14)
> Why was this even made a platform ifdef at all? Is there any platform where 1 << 15 is worse than 1 << 16?

- As far as I experimented locally, 1<<15 is better than 1<<16 in Chromium/Mac. 1<<15 and 1<<16 are the same in AppleWebKit/Mac and Chromium/Linux (their performance "gap" exists at between 1<<17 and 1<<18).
- According to anttik (who wrote 1<<16 a long time ago), there was no strong reason for 1<<16.
- The reason why I changed 1<<16 to 1<<15 on Chromium/Mac only is that it seems the value highly depends on malloc systems and I was afraid that changing to 1<<15 _might_ cause performance regression in some platform.

But as you pointed, I think 1<<15 would not be worse than 1<<16 in all platforms. I'll change it to 1<<15 in a following patch.
------- Comment #17 From 2012-03-18 04:48:36 PST -------
Reopening to attach new patch.
------- Comment #18 From 2012-03-18 04:48:40 PST -------
Created an attachment (id=132490) [details]
Patch
------- Comment #19 From 2012-03-18 10:00:03 PST -------
(From update of attachment 132490 [details])
Clearing flags on attachment: 132490

Committed r111133: <http://trac.webkit.org/changeset/111133>
------- Comment #20 From 2012-03-18 10:00:10 PST -------
All reviewed patches have been landed.  Closing bug.