Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1552793002:
CompositorWorker: Use a linear search in compositorMutablePropertyForName. (Closed)
|
Created:
4 years, 11 months ago by jbroman Modified:
4 years, 11 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCompositorWorker: Use a linear search in compositorMutablePropertyForName.
It doesn't incur any allocations, is simple, and handles 16-bit strings
well. Since the number of supported properties is so small, a linear search
is quite fast.
BUG=531577
Committed: https://crrev.com/ee750771ee64d8171670d6d6d525e71378bd2adb
Cr-Commit-Position: refs/heads/master@{#367170}
Patch Set 1 #Patch Set 2 : case-insensitive #Patch Set 3 : add test which creates a 16-bit string for a valid property #
Total comments: 2
Messages
Total messages: 20 (7 generated)
jbroman@chromium.org changed reviewers: + vollick@chromium.org
The linear search we discussed. While here, I made the mapping const (so it can go in read-only memory). I don't think this really demands additional tests, but let me know if you feel otherwise.
On 2015/12/30 16:38:27, jbroman wrote:
> The linear search we discussed. While here, I made the mapping const (so it
can
> go in read-only memory).
>
> I don't think this really demands additional tests, but let me know if you
feel
> otherwise.
Thanks! Bug fix aside, I like this straightforward approach much better.
I do think we should get a bit more coverage for this, and you effectively
described how to get that coverage in your comments on the bug :)
You could add the following to compositor-proxy-supports.html.
var transform16 = new TextDecoder('utf-16').decode(new
TextEncoder('utf-16').encode('transform'));
assert_true(transform16 === 'transform');
assert_true(proxy.supports(transform16));
On 2015/12/30 at 16:50:15, vollick wrote:
> On 2015/12/30 16:38:27, jbroman wrote:
> > The linear search we discussed. While here, I made the mapping const (so it
can
> > go in read-only memory).
> >
> > I don't think this really demands additional tests, but let me know if you
feel
> > otherwise.
>
> Thanks! Bug fix aside, I like this straightforward approach much better.
>
> I do think we should get a bit more coverage for this, and you effectively
described how to get that coverage in your comments on the bug :)
>
> You could add the following to compositor-proxy-supports.html.
>
> var transform16 = new TextDecoder('utf-16').decode(new
TextEncoder('utf-16').encode('transform'));
> assert_true(transform16 === 'transform');
> assert_true(proxy.supports(transform16));
Done.
The CQ bit was checked by vollick@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1552793002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1552793002/40001
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
I'm pretty sure this is a JS bindings bug with the string cache, we should have used the 8bit storage for that string. We can switch to linear search if you want though, but please file the storage bug too. https://codereview.chromium.org/1552793002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-supports.html (right): https://codereview.chromium.org/1552793002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-supports.html:42: // This is a hack to get a 16-bit string for a supported property. Our strings should select the latin1 storage for ascii only strings, this seems like a JS bindings bug. https://codereview.chromium.org/1552793002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (left): https://codereview.chromium.org/1552793002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/CompositorProxy.cpp:44: const String attributeLower = attributeName.lower(); The only bug here is that this doesn't do .utf8().data() and use the CString.
The CQ bit was unchecked by jbroman@chromium.org
On 2015/12/30 at 18:31:30, esprehn wrote: > I'm pretty sure this is a JS bindings bug with the string cache, we should have used the 8bit storage for that string. > > We can switch to linear search if you want though, but please file the storage bug too. > > https://codereview.chromium.org/1552793002/diff/40001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-supports.html (right): > > https://codereview.chromium.org/1552793002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-supports.html:42: // This is a hack to get a 16-bit string for a supported property. > Our strings should select the latin1 storage for ascii only strings, this seems like a JS bindings bug. I don't think that's supposed to be an invariant; we _can_ select it, and often do, but I don't think code is supposed to assume it. The equality operator for strings, for example, explicitly handles character-by-character equality when one string is 8-bit and the other is 16-bit. If our strings promised never to use larger storage than necessary, that wouldn't be so. abarth's document on WTF strings (https://docs.google.com/document/d/1kOCUlJdh2WJMJGDf-WoEQhmnjKLaOYRbiHz5TiGJl...) also only suggests that one "consider" using create8BitIfPossible in places where we might be able to convert a 16-bit string to 8-bit, but notes that there's an associated cost. > https://codereview.chromium.org/1552793002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (left): > > https://codereview.chromium.org/1552793002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/CompositorProxy.cpp:44: const String attributeLower = attributeName.lower(); > The only bug here is that this doesn't do .utf8().data() and use the CString. That, or using StringUTF8Adaptor (which would be more efficient), would solve the 16-bit string bug, but not the other (ordering) bug.
On 2015/12/30 at 18:38:15, jbroman wrote: > That, or using StringUTF8Adaptor (which would be more efficient), would solve the 16-bit string bug, but not the other (ordering) bug. Oh, I suppose if the resulting CString were used with strcmp instead of memcmp, that would fix the ordering issue as well, at the cost of up to two heap allocations.
The CQ bit was checked by jbroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1552793002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1552793002/40001
Message was sent while issue was closed.
It sounds like you didn't object to this CL in particular, so resuming CQ.
The CQ bit was checked by jbroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1552793002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1552793002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== CompositorWorker: Use a linear search in compositorMutablePropertyForName. It doesn't incur any allocations, is simple, and handles 16-bit strings well. Since the number of supported properties is so small, a linear search is quite fast. BUG=531577 ========== to ========== CompositorWorker: Use a linear search in compositorMutablePropertyForName. It doesn't incur any allocations, is simple, and handles 16-bit strings well. Since the number of supported properties is so small, a linear search is quite fast. BUG=531577 Committed: https://crrev.com/ee750771ee64d8171670d6d6d525e71378bd2adb Cr-Commit-Position: refs/heads/master@{#367170} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ee750771ee64d8171670d6d6d525e71378bd2adb Cr-Commit-Position: refs/heads/master@{#367170} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
