Reland natural implementation of (explicit) RefPtr::operator bool. |
|||||||||||||
Issue descriptionSplitting this off 605080 to be clear the regression is solved. By splitting the CL into ~six small pieces, it seems that it's the change to give PassRefPtr (not RefPtr) an explicit operator bool that's the issue. Still not sure _WHY_, though.
,
Apr 27 2016
The implementation used none of those; it just used a direct conversion. It was the call sites that couldn't transition to the explicit operator that got .get(). I ran the perf tests against just those changes, with no effect. So far, I've bisected it down to "some call site of PassRefPtr<T>::operator UnspecifiedBoolType". I've got a couple shots in the dark running on the bots now.
,
Apr 27 2016
Misread a test result; it's RefPtr<T>, but PassRefPtr<T>, that causes this issue. Which is a shame, because RefPtr<T> is the one with way more call sites. :(
,
Apr 28 2016
,
Apr 28 2016
Update: There is no regression if the explicit operator bool still does "m_ptr ? &RefPtr::m_ptr : 0", so at least it's not the fact that it's an explicit conversion operator that's causing problems. My best guess is that the compiler makes a bad optimization (inlining?) decision when the function is simpler, but I can't figure out where.
,
May 3 2016
ARM disassembly of both versions: https://gist.github.com/jeremyroman/e64d2ee6cd7ffee0b40d104af0db84a9 The blocks end up in a weirder order in the pre-patch (master) version, but more importantly, the tight loop (the source of which wasn't changed!) winds up one instruction and one branch shorter than after the patch. I attribute that "something was slightly different in the optimizer state", which isn't very actionable. I'm going to try replacing the 8-bit version with memchr, which is probably optimized to be even faster.
,
May 4 2016
,
May 4 2016
,
May 8 2016
,
May 12 2016
,
Jun 2 2016
Re-summary: this is not an issue with explicit conversion operators, but with how arm gcc mis-optimized this particular case when the implementation changed from "m_ptr ? &RefPtr::m_ptr : 0" to "m_ptr". I don't see any need to block changing of other places to explicit operator bool; this only indicates that RefPtr-to-bool conversion has one particularly sensitive call site.
,
Jun 2 2016
,
Jun 3 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c9f9af30569ac2cd353e234f569052db6ab436f4 commit c9f9af30569ac2cd353e234f569052db6ab436f4 Author: jbroman <jbroman@chromium.org> Date: Fri Jun 03 00:53:22 2016 Switch WTF::find on LChar to use memchr. BUG= 607208 Review-Url: https://codereview.chromium.org/1948543004 Cr-Commit-Position: refs/heads/master@{#397568} [modify] https://crrev.com/c9f9af30569ac2cd353e234f569052db6ab436f4/third_party/WebKit/Source/wtf/text/StringImpl.h
,
Jun 3 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/08438794f10a999c095ad758cae293c7a1beff0f commit 08438794f10a999c095ad758cae293c7a1beff0f Author: fs <fs@opera.com> Date: Fri Jun 03 08:23:43 2016 Revert of Switch WTF::find on LChar to use memchr. (patchset #1 id:1 of https://codereview.chromium.org/1948543004/ ) Reason for revert: 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_ASAN/24421/layout-test-results/results.html) Original issue's description: > Switch WTF::find on LChar to use memchr. > > BUG= 607208 > > Committed: https://crrev.com/c9f9af30569ac2cd353e234f569052db6ab436f4 > Cr-Commit-Position: refs/heads/master@{#397568} TBR=thakis@chromium.org,jbroman@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 607208 Review-Url: https://codereview.chromium.org/2036993002 Cr-Commit-Position: refs/heads/master@{#397664} [modify] https://crrev.com/08438794f10a999c095ad758cae293c7a1beff0f/third_party/WebKit/Source/wtf/text/StringImpl.h
,
Jun 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c1ddfc2c184e332cbb6796ec5b28040ad3a3b8d4 commit c1ddfc2c184e332cbb6796ec5b28040ad3a3b8d4 Author: jbroman <jbroman@chromium.org> Date: Fri Jun 10 22:33:35 2016 Reland: Switch WTF::find on LChar to use memchr. BUG= 607208 Review-Url: https://codereview.chromium.org/2043063003 Cr-Commit-Position: refs/heads/master@{#399300} [modify] https://crrev.com/c1ddfc2c184e332cbb6796ec5b28040ad3a3b8d4/third_party/WebKit/Source/wtf/text/StringImpl.h
,
Jun 13 2016
Performance results: https://chromeperf.appspot.com/group_report?rev=399300 (but I believe the smoothness and thread_times changes to be unrelated): Highlights: - massive progression on blink_perf.dom:select-long-word (ranging from about 15% on N6 to 90% on N5X -- the difference between 800 ms and 80 ms) - notable progressions on blink_perf.dom:textarea-dom (10-30%) and blink_perf.parser:textarea-parsing (10%) blink_perf.dom:select-multiple-add shows up to 24% regression on some bots which could be related to this patch, but I'm not confident that it is, because unlike the progression, it doesn't show a change with the original landing/revert (around 397600). And such a change did not manifest at all on my local Nexus 4. Even if it does track to my patch, the effect on select-long-word is, IMHO, much more significant. Also, keeping this patch unblocks the RefPtr::operator bool rewrite.
,
Jun 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c1ddfc2c184e332cbb6796ec5b28040ad3a3b8d4 commit c1ddfc2c184e332cbb6796ec5b28040ad3a3b8d4 Author: jbroman <jbroman@chromium.org> Date: Fri Jun 10 22:33:35 2016 Reland: Switch WTF::find on LChar to use memchr. BUG= 607208 Review-Url: https://codereview.chromium.org/2043063003 Cr-Commit-Position: refs/heads/master@{#399300} [modify] https://crrev.com/c1ddfc2c184e332cbb6796ec5b28040ad3a3b8d4/third_party/WebKit/Source/wtf/text/StringImpl.h
,
Jun 15 2016
,
Jun 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/70843029b30a2566640848f072312cde61954ed3 commit 70843029b30a2566640848f072312cde61954ed3 Author: jbroman <jbroman@chromium.org> Date: Wed Jun 15 18:54:50 2016 Reland: WTF: Implement explicit RefPtr::operator bool. This is the modern alternative to the "safe bool" idiom. Some changes at call sites are required, primarily because returning a boolean does not cause an explicit conversion. This is a reland of https://codereview.chromium.org/1891473002/ with no changes aside from a rebase. There should be no more performance impact after the fix in bug 607208 . BUG= 607208 Review-Url: https://codereview.chromium.org/2063773002 Cr-Commit-Position: refs/heads/master@{#399979} [modify] https://crrev.com/70843029b30a2566640848f072312cde61954ed3/third_party/WebKit/Source/core/css/LocalFontFaceSource.cpp [modify] https://crrev.com/70843029b30a2566640848f072312cde61954ed3/third_party/WebKit/Source/core/fetch/SubstituteData.h [modify] https://crrev.com/70843029b30a2566640848f072312cde61954ed3/third_party/WebKit/Source/core/inspector/NetworkResourcesData.h [modify] https://crrev.com/70843029b30a2566640848f072312cde61954ed3/third_party/WebKit/Source/core/layout/LayoutCounter.cpp [modify] https://crrev.com/70843029b30a2566640848f072312cde61954ed3/third_party/WebKit/Source/core/style/ComputedStyle.h [modify] https://crrev.com/70843029b30a2566640848f072312cde61954ed3/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp [modify] https://crrev.com/70843029b30a2566640848f072312cde61954ed3/third_party/WebKit/Source/platform/PODRedBlackTree.h [modify] https://crrev.com/70843029b30a2566640848f072312cde61954ed3/third_party/WebKit/Source/platform/fonts/SimpleFontData.h [modify] https://crrev.com/70843029b30a2566640848f072312cde61954ed3/third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h [modify] https://crrev.com/70843029b30a2566640848f072312cde61954ed3/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp [modify] https://crrev.com/70843029b30a2566640848f072312cde61954ed3/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.h [modify] https://crrev.com/70843029b30a2566640848f072312cde61954ed3/third_party/WebKit/Source/platform/graphics/paint/DrawingDisplayItem.cpp [modify] https://crrev.com/70843029b30a2566640848f072312cde61954ed3/third_party/WebKit/Source/web/WebViewImpl.h [modify] https://crrev.com/70843029b30a2566640848f072312cde61954ed3/third_party/WebKit/Source/wtf/PassRefPtr.h [modify] https://crrev.com/70843029b30a2566640848f072312cde61954ed3/third_party/WebKit/Source/wtf/RefPtr.h [modify] https://crrev.com/70843029b30a2566640848f072312cde61954ed3/third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferBuilder.h [modify] https://crrev.com/70843029b30a2566640848f072312cde61954ed3/third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferView.h
,
Jun 16 2016
Whoops, I thought I'd used the final implementation of operator bool in that CL, but apparently I uploaded at the wrong commit. Stand by. :)
,
Jun 17 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2b6875bfd954d336432702512d1ffe4c83343d2b commit 2b6875bfd954d336432702512d1ffe4c83343d2b Author: jbroman <jbroman@chromium.org> Date: Fri Jun 17 00:36:02 2016 WTF: Simplify RefPtr::operator bool. The problematic case in String::find on ARM should now be gone, and so this should be both simpler and equally fast. BUG= 607208 Review-Url: https://codereview.chromium.org/2072703002 Cr-Commit-Position: refs/heads/master@{#400315} [modify] https://crrev.com/2b6875bfd954d336432702512d1ffe4c83343d2b/third_party/WebKit/Source/wtf/PassRefPtr.h [modify] https://crrev.com/2b6875bfd954d336432702512d1ffe4c83343d2b/third_party/WebKit/Source/wtf/RefPtr.h
,
Jun 17 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2862dfadaa7fc3b96a2f56facb00ef736fbe70d9 commit 2862dfadaa7fc3b96a2f56facb00ef736fbe70d9 Author: dalecurtis <dalecurtis@chromium.org> Date: Fri Jun 17 01:20:38 2016 Revert of WTF: Simplify RefPtr::operator bool. (patchset #1 id:1 of https://codereview.chromium.org/2072703002/ ) Reason for revert: Broke x64 build, see original patch for details. Original issue's description: > WTF: Simplify RefPtr::operator bool. > > The problematic case in String::find on ARM should now be gone, and so this > should be both simpler and equally fast. > > BUG= 607208 > > Committed: https://crrev.com/2b6875bfd954d336432702512d1ffe4c83343d2b > Cr-Commit-Position: refs/heads/master@{#400315} TBR=thakis@chromium.org,jbroman@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 607208 Review-Url: https://codereview.chromium.org/2071183002 Cr-Commit-Position: refs/heads/master@{#400325} [modify] https://crrev.com/2862dfadaa7fc3b96a2f56facb00ef736fbe70d9/third_party/WebKit/Source/wtf/PassRefPtr.h [modify] https://crrev.com/2862dfadaa7fc3b96a2f56facb00ef736fbe70d9/third_party/WebKit/Source/wtf/RefPtr.h
,
Jun 17 2016
Ugh, yet another orthogonal issue. Will file shortly and block on this.
,
Jun 17 2016
Renaming the bug because the cause was investigated.
,
Jun 17 2016
,
Jun 23 2016
Renaming Blink>WTF to Blink>Internals>WTF.
,
Jun 24 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4db41c4c92314447a07a2255d97c344547f97ce7 commit 4db41c4c92314447a07a2255d97c344547f97ce7 Author: jbroman <jbroman@chromium.org> Date: Fri Jun 24 17:40:51 2016 Reland: WTF: Simplify RefPtr::operator bool. The problematic case in String::find on ARM should now be gone, and so this should be both simpler and equally fast. Comparing to nullptr is used to coerce to boolean to work around bug 621177 (even though this is usually not the preferred way to do this in Blink). BUG= 607208 , 621177 Review-Url: https://codereview.chromium.org/2090973002 Cr-Commit-Position: refs/heads/master@{#401897} [modify] https://crrev.com/4db41c4c92314447a07a2255d97c344547f97ce7/third_party/WebKit/Source/wtf/PassRefPtr.h [modify] https://crrev.com/4db41c4c92314447a07a2255d97c344547f97ce7/third_party/WebKit/Source/wtf/RefPtr.h
,
Jun 28 2016
|
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by danakj@chromium.org
, Apr 27 2016