Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2043063003:
Reland: Switch WTF::find on LChar to use memchr. (Closed)
|
Created:
4 years, 6 months ago by jbroman Modified:
4 years, 6 months ago Reviewers:
Nico CC:
blink-reviews, blink-reviews-wtf_chromium.org, chromium-reviews, Mikhail Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReland: Switch WTF::find on LChar to use memchr.
BUG=607208
Committed: https://crrev.com/c1ddfc2c184e332cbb6796ec5b28040ad3a3b8d4
Cr-Commit-Position: refs/heads/master@{#399300}
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Messages
Total messages: 14 (3 generated)
jbroman@chromium.org changed reviewers: + thakis@chromium.org
thakis, PTAL. This CL is a reland of https://codereview.chromium.org/1948543004 It turns out some call sites expect to be able to pass an out-of-range index and get kNotFound in response. With this additional check, the MSAN failure goes away, and I believe this function to now be totally equivalent in functionality. Performance gains are maintained. blink_perf.dom on my Nexus 4 (ARM): div-editable: Equal modify-element-classname: Equal modify-element-id: Equal modify-element-title: 1.73% Worse select-long-word: 19.33% Better select-multiple-add: 1.23% Better select-single-add: Equal textarea-dom: 7.49% Better textarea-edit: Equal (and, of course, avoids the ~13% regression on select-long-word with the RefPtr change added) I've tried conditionally using memchr (only for strings longer than some fixed length), and while it does improve a couple tests further (notably textarea-dom), on the whole it seems to be better to just use memchr, at least for blink_perf.dom on my Nexus 4.
ping
lgtm I knew it! Usual procedure for uploading fixed patches is to upload patch-as-is in patch set 1, and then with fix in patch set 2 https://codereview.chromium.org/2043063003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/StringImpl.h (right): https://codereview.chromium.org/2043063003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringImpl.h:532: // Some clients rely on being able to pass index >= length. should the clients be fixed instead?
The CQ bit was checked by jbroman@chromium.org
Ah, sorry for not including the original patch, but you quickly found the 3 lines I added. :) https://codereview.chromium.org/2043063003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/text/StringImpl.h (right): https://codereview.chromium.org/2043063003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/text/StringImpl.h:532: // Some clients rely on being able to pass index >= length. On 2016/06/10 at 19:09:14, Nico (traveling...slow) wrote: > should the clients be fixed instead? In my ideal world (in which nobody passes out-of-range indexes), yes. But I fear such sites are subtle and hard to find, except when MSAN happens to find them. The particular one that was reported was in ContentType::parameter, where start+1 and start+2 are passed without bounds checking. It's fixable there, but I suspect other sites also rely on it, and there are many to inspect. I don't think it's worth doing as part of this wild goose chase, since it's not super harmful (at least, now that I know that such sites exist).
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2043063003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Reland: Switch WTF::find on LChar to use memchr. BUG=607208 ========== to ========== Reland: Switch WTF::find on LChar to use memchr. BUG=607208 Committed: https://crrev.com/c1ddfc2c184e332cbb6796ec5b28040ad3a3b8d4 Cr-Commit-Position: refs/heads/master@{#399300} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c1ddfc2c184e332cbb6796ec5b28040ad3a3b8d4 Cr-Commit-Position: refs/heads/master@{#399300}
Message was sent while issue was closed.
In the future please write more detailed change descriptions, why was this a good change? What impact does it have? :)
Message was sent while issue was closed.
My apologies. I did explain some of that in the bug and my initial review comments, but I can see wanting more of it in the commit description as well. On Sun, Jun 12, 2016, 7:03 PM <esprehn@chromium.org> wrote: > In the future please write more detailed change descriptions, why was this > a > good change? What impact does it have? :) > > https://codereview.chromium.org/2043063003/ > -- 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.
Message was sent while issue was closed.
My apologies. I did explain some of that in the bug and my initial review comments, but I can see wanting more of it in the commit description as well. On Sun, Jun 12, 2016, 7:03 PM <esprehn@chromium.org> wrote: > In the future please write more detailed change descriptions, why was this > a > good change? What impact does it have? :) > > https://codereview.chromium.org/2043063003/ > -- 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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
