WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Patch
(6.18 KB, patch)
2012-03-14 23:04 PDT
,
Kentaro Hara
no flags
Details
Formatted Diff
Diff
Updated performance test
(802 bytes, text/html)
2012-03-15 00:04 PDT
,
Kentaro Hara
no flags
Details
Patch
(6.88 KB, patch)
2012-03-15 00:09 PDT
,
Kentaro Hara
no flags
Details
Formatted Diff
Diff
Patch
(3.74 KB, patch)
2012-03-15 07:00 PDT
,
Kentaro Hara
no flags
Details
Formatted Diff
Diff
Patch
(3.79 KB, patch)
2012-03-15 21:12 PDT
,
Kentaro Hara
no flags
Details
Formatted Diff
Diff
Patch
(2.63 KB, patch)
2012-03-18 04:48 PDT
,
Kentaro Hara
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Kentaro Hara
Comment 1
2012-03-14 23:04:24 PDT
Created
attachment 131990
[details]
Patch
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
Created
attachment 131995
[details]
Patch
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
Created
attachment 132038
[details]
Patch
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
Created
attachment 132190
[details]
Patch
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
Created
attachment 132490
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug