Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1948543004:
Switch WTF::find on LChar to use memchr. (Closed)
|
Created:
4 years, 7 months ago by jbroman Modified:
4 years, 6 months ago Reviewers:
Nico CC:
blink-reviews, blink-reviews-wtf_chromium.org, chromium-reviews, danakj, Mikhail Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitch WTF::find on LChar to use memchr.
BUG=607208
Committed: https://crrev.com/c9f9af30569ac2cd353e234f569052db6ab436f4
Cr-Commit-Position: refs/heads/master@{#397568}
Patch Set 1 #
Total comments: 4
Messages
Total messages: 19 (4 generated)
jbroman@chromium.org changed reviewers: + thakis@chromium.org
The slowness referred to in bug 605080 is caused by the loop in this WTF::find being optimized poorly when pre-optimization code outside the loop changes slightly. Changing to libc's memchr here sidesteps that issue, and net improves performance in some perf tests that rely on lots of searches of this type (especially those that search long strings -- memchr is typically more optimized, especially for those). In particular, a few tests (notably blink_perf.dom:select-long-word) progress about 13%. Then there's a roughly even mixture of progressions and regressions, mostly around 2%, a few around 5%. This seems to be a net win on the whole, and also prevents landing the natural implementation of RefPtr::operator bool from causing a regression on blink_perf.dom. WDYT?
ping
sorry. BUG=607208, right? memchr will be a call. For longer strings, this should be faster, but for shorter strings it won't be. It sounds like we have good benchmarks coverage for this, so worth a try I guess :-) https://codereview.chromium.org/1948543004/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/text/StringImpl.h (right): https://codereview.chromium.org/1948543004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/text/StringImpl.h:525: memchr(characters + index, matchCharacter, length - index)); what if index > length? https://codereview.chromium.org/1948543004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/text/StringImpl.h:554: return find(characters, length, static_cast<LChar>(matchCharacter), index); why cast matchCharacter to LChar?
Description was changed from ========== Switch WTF::find on LChar to use memchr. BUG=605080 ========== to ========== Switch WTF::find on LChar to use memchr. BUG=607208 ==========
On 2016/05/10 at 17:47:06, thakis wrote: > sorry. > > BUG=607208, right? Sure. > memchr will be a call. For longer strings, this should be faster, but for shorter strings it won't be. It sounds like we have good benchmarks coverage for this, so worth a try I guess :-) I could do a check if length is larger than $ARBITRARY_CUTOFF, though that also has overhead (cost of the check, cost of larger function, may affect inlining, etc.). I haven't experimented with that. When I ran benchmarks, I saw some progressions and some regressions from this patch, but on the whole it seemed a win to me: https://codereview.chromium.org/1948543004#msg2 > https://codereview.chromium.org/1948543004/diff/1/third_party/WebKit/Source/w... > File third_party/WebKit/Source/wtf/text/StringImpl.h (right): > > https://codereview.chromium.org/1948543004/diff/1/third_party/WebKit/Source/w... > third_party/WebKit/Source/wtf/text/StringImpl.h:525: memchr(characters + index, matchCharacter, length - index)); > what if index > length? > > https://codereview.chromium.org/1948543004/diff/1/third_party/WebKit/Source/w... > third_party/WebKit/Source/wtf/text/StringImpl.h:554: return find(characters, length, static_cast<LChar>(matchCharacter), index); > why cast matchCharacter to LChar?
(you didn't reply to the in-code comments) On Tue, May 10, 2016 at 7:45 PM, <jbroman@chromium.org> wrote: > On 2016/05/10 at 17:47:06, thakis wrote: > > sorry. > > > > BUG=607208, right? > > Sure. > > > memchr will be a call. For longer strings, this should be faster, but for > shorter strings it won't be. It sounds like we have good benchmarks > coverage for > this, so worth a try I guess :-) > > I could do a check if length is larger than $ARBITRARY_CUTOFF, though that > also > has overhead (cost of the check, cost of larger function, may affect > inlining, > etc.). I haven't experimented with that. > > When I ran benchmarks, I saw some progressions and some regressions from > this > patch, but on the whole it seemed a win to me: > https://codereview.chromium.org/1948543004#msg2 > > > > > https://codereview.chromium.org/1948543004/diff/1/third_party/WebKit/Source/w... > > File third_party/WebKit/Source/wtf/text/StringImpl.h (right): > > > > > > https://codereview.chromium.org/1948543004/diff/1/third_party/WebKit/Source/w... > > third_party/WebKit/Source/wtf/text/StringImpl.h:525: memchr(characters + > index, matchCharacter, length - index)); > > what if index > length? > > > > > > https://codereview.chromium.org/1948543004/diff/1/third_party/WebKit/Source/w... > > third_party/WebKit/Source/wtf/text/StringImpl.h:554: return > find(characters, > length, static_cast<LChar>(matchCharacter), index); > > why cast matchCharacter to LChar? > > > https://codereview.chromium.org/1948543004/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
(you didn't reply to the in-code comments) On Tue, May 10, 2016 at 7:45 PM, <jbroman@chromium.org> wrote: > On 2016/05/10 at 17:47:06, thakis wrote: > > sorry. > > > > BUG=607208, right? > > Sure. > > > memchr will be a call. For longer strings, this should be faster, but for > shorter strings it won't be. It sounds like we have good benchmarks > coverage for > this, so worth a try I guess :-) > > I could do a check if length is larger than $ARBITRARY_CUTOFF, though that > also > has overhead (cost of the check, cost of larger function, may affect > inlining, > etc.). I haven't experimented with that. > > When I ran benchmarks, I saw some progressions and some regressions from > this > patch, but on the whole it seemed a win to me: > https://codereview.chromium.org/1948543004#msg2 > > > > > https://codereview.chromium.org/1948543004/diff/1/third_party/WebKit/Source/w... > > File third_party/WebKit/Source/wtf/text/StringImpl.h (right): > > > > > > https://codereview.chromium.org/1948543004/diff/1/third_party/WebKit/Source/w... > > third_party/WebKit/Source/wtf/text/StringImpl.h:525: memchr(characters + > index, matchCharacter, length - index)); > > what if index > length? > > > > > > https://codereview.chromium.org/1948543004/diff/1/third_party/WebKit/Source/w... > > third_party/WebKit/Source/wtf/text/StringImpl.h:554: return > find(characters, > length, static_cast<LChar>(matchCharacter), index); > > why cast matchCharacter to LChar? > > > https://codereview.chromium.org/1948543004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry -- I wrote replies, but forgot that "reply" doesn't publish drafts. :( https://codereview.chromium.org/1948543004/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/text/StringImpl.h (right): https://codereview.chromium.org/1948543004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/text/StringImpl.h:525: memchr(characters + index, matchCharacter, length - index)); On 2016/05/10 at 17:47:06, Nico wrote: > what if index > length? I'd expect that to be an error at the call site (it seems equivalent to indexing past the end of the array). But I can check it, if you'd like. https://codereview.chromium.org/1948543004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/text/StringImpl.h:554: return find(characters, length, static_cast<LChar>(matchCharacter), index); On 2016/05/10 at 17:47:06, Nico wrote: > why cast matchCharacter to LChar? To call the actual implementation above. Without the cast, this function calls itself.
ping?
ping?
lgtm Sorry, this fell out my inbox somehow.
The CQ bit was checked by jbroman@chromium.org
/me crosses his fingers
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1948543004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1948543004/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Switch WTF::find on LChar to use memchr. BUG=607208 ========== to ========== Switch WTF::find on LChar to use memchr. BUG=607208 Committed: https://crrev.com/c9f9af30569ac2cd353e234f569052db6ab436f4 Cr-Commit-Position: refs/heads/master@{#397568} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/c9f9af30569ac2cd353e234f569052db6ab436f4 Cr-Commit-Position: refs/heads/master@{#397568}
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2036993002/ by fs@opera.com. The reason for reverting is: LSAN and MSAN bots appear unhappy: http/tests/media/media-source/mediasource-is-type-supported.html crash log for renderer (pid <unknown>): STDOUT: <empty> STDERR: ================================================================= STDERR: ==4==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6030000982af at pc 0x00000045811f bp 0x7fff2f309830 sp 0x7fff2f308fe0 STDERR: READ of size 5006 at 0x6030000982af thread T0 (content_shell) STDERR: #0 0x45811e in memchr ??:0 STDERR: #1 0x3c5c419 in find third_party/WebKit/Source/wtf/text/StringImpl.h:532:9 STDERR: #2 0x3c5c419 in find third_party/WebKit/Source/wtf/text/StringImpl.h:660:0 STDERR: #3 0x3c5c419 in find third_party/WebKit/Source/wtf/text/WTFString.h:214:0 STDERR: #4 0x3c5c419 in find third_party/WebKit/Source/wtf/text/WTFString.h:215:0 STDERR: #5 0x3c5c419 in parameter third_party/WebKit/Source/platform/ContentType.cpp:50:0 STDERR: #6 0x8d64b7d in isTypeSupported third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:244:33 STDERR: #7 0x9251198 in isTypeSupportedMethod ./out/Release/gen/blink/bindings/modules/v8/V8MediaSource.cpp:234:32 STDERR: #8 0x9251198 in isTypeSupportedMethodCallback ./out/Release/gen/blink/bindings/modules/v8/V8MediaSource.cpp:239:0 STDERR: #9 0x444b759 in Call v8/src/api-arguments.cc:16:3 (https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_ASA...). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
