New issue
Advanced search Search tips

Issue 607208 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocked on:
issue 605080
issue 621177



Sign in to add a comment

Reland natural implementation of (explicit) RefPtr::operator bool.

Project Member Reported by jbroman@chromium.org, Apr 27 2016

Issue description

Splitting 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.
 

Comment 1 by danakj@chromium.org, Apr 27 2016

What if you change its implementation to be like !! or static_cast instead of .get()
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.
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. :(

Comment 4 by yutak@chromium.org, Apr 28 2016

Components: -Blink Blink>WTF
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.
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.

Comment 7 by w...@chromium.org, May 4 2016

Cc: w...@chromium.org

Comment 8 by w...@chromium.org, May 4 2016

Blocking: 605794
Blocking: 610048
Blocking: -610048
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.

Comment 12 by w...@chromium.org, Jun 2 2016

Blocking: -605794
Project Member

Comment 13 by bugdroid1@chromium.org, 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

Project Member

Comment 14 by bugdroid1@chromium.org, 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

Project Member

Comment 15 by bugdroid1@chromium.org, 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

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.
Project Member

Comment 17 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Project Member

Comment 19 by bugdroid1@chromium.org, 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

Status: Assigned (was: Fixed)
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. :)
Project Member

Comment 21 by bugdroid1@chromium.org, 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

Project Member

Comment 22 by bugdroid1@chromium.org, 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

Ugh, yet another orthogonal issue. Will file shortly and block on this.
Summary: Reland natural implementation of (explicit) RefPtr::operator bool. (was: Investigate cause of regression 605080 (due to PassRefPtr/RefPtr operator bool))
Renaming the bug because the cause was investigated.
Blockedon: 621177

Comment 26 by tkent@chromium.org, Jun 23 2016

Components: -Blink>WTF Blink>Internals>WTF
Renaming Blink>WTF to Blink>Internals>WTF.

Project Member

Comment 27 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)

Sign in to add a comment