RESOLVED FIXED 81192
[Chromium][Performance] Optimize innerText and outerText in Chromium/Mac
https://bugs.webkit.org/show_bug.cgi?id=81192
Summary [Chromium][Performance] Optimize innerText and outerText in Chromium/Mac
Kentaro Hara
Reported 2012-03-14 23:00:49 PDT
Created attachment 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
Attachments
Performance test (542 bytes, text/html)
2012-03-14 23:00 PDT, Kentaro Hara
no flags
Patch (6.18 KB, patch)
2012-03-14 23:04 PDT, Kentaro Hara
no flags
Updated performance test (802 bytes, text/html)
2012-03-15 00:04 PDT, Kentaro Hara
no flags
Patch (6.88 KB, patch)
2012-03-15 00:09 PDT, Kentaro Hara
no flags
Patch (3.74 KB, patch)
2012-03-15 07:00 PDT, Kentaro Hara
no flags
Patch (3.79 KB, patch)
2012-03-15 21:12 PDT, Kentaro Hara
no flags
Patch (2.63 KB, patch)
2012-03-18 04:48 PDT, Kentaro Hara
no flags
Kentaro Hara
Comment 1 2012-03-14 23:04:24 PDT
Kentaro Hara
Comment 2 2012-03-15 00:04:26 PDT
Created attachment 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
Kentaro Hara
Comment 3 2012-03-15 00:09:49 PDT
Antti Koivisto
Comment 4 2012-03-15 04:43:45 PDT
Comment on attachment 131995 [details] Patch 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.
Kentaro Hara
Comment 5 2012-03-15 07:00:32 PDT
Kentaro Hara
Comment 6 2012-03-15 07:14:20 PDT
(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.
Dimitri Glazkov (Google)
Comment 7 2012-03-15 10:25:25 PDT
Comment on attachment 132038 [details] Patch 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?
Dai Mikurube
Comment 8 2012-03-15 13:33:35 PDT
(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.
Kentaro Hara
Comment 9 2012-03-15 17:39:19 PDT
Comment on attachment 132038 [details] Patch Thanks for comments. Let me invalidate r? for now to investigate more. A new patch is coming soon.
Kentaro Hara
Comment 10 2012-03-15 21:12:51 PDT
Kentaro Hara
Comment 11 2012-03-15 21:16:53 PDT
(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)".
WebKit Review Bot
Comment 12 2012-03-16 19:33:07 PDT
Comment on attachment 132190 [details] Patch Clearing flags on attachment: 132190 Committed r111109: <http://trac.webkit.org/changeset/111109>
WebKit Review Bot
Comment 13 2012-03-16 19:33:12 PDT
All reviewed patches have been landed. Closing bug.
Maciej Stachowiak
Comment 14 2012-03-17 19:17:08 PDT
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.
Dimitri Glazkov (Google)
Comment 15 2012-03-17 19:20:35 PDT
(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.
Kentaro Hara
Comment 16 2012-03-17 20:34:05 PDT
(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.
Kentaro Hara
Comment 17 2012-03-18 04:48:36 PDT
Reopening to attach new patch.
Kentaro Hara
Comment 18 2012-03-18 04:48:40 PDT
WebKit Review Bot
Comment 19 2012-03-18 10:00:03 PDT
Comment on attachment 132490 [details] Patch Clearing flags on attachment: 132490 Committed r111133: <http://trac.webkit.org/changeset/111133>
WebKit Review Bot
Comment 20 2012-03-18 10:00:10 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.