Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2961413003:
Outline RefCountedThreadSafeBase::AddRef and Release on non-X86 (Closed)
DescriptionOutline RefCountedThreadSafeBase::AddRef and Release on non-X86
They were made inline in #481091, which caused a size drop on X86 and
size increase on ARM.
The code sequence on ARM is larger than on X86, and probably not worth
inlining.
BUG=728324, 736015
Review-Url: https://codereview.chromium.org/2961413003
Cr-Commit-Position: refs/heads/master@{#483759}
Committed: https://chromium.googlesource.com/chromium/src/+/79eb7ebaabeb87fd76da56f35de0ab5776806a15
Patch Set 1 #
Total comments: 2
Messages
Total messages: 20 (10 generated)
hans@chromium.org changed reviewers: + agrieve@chromium.org, thakis@chromium.org
What do you think?
The CQ bit was checked by hans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thakis@google.com changed reviewers: + thakis@google.com
lgtm, let's see if this reduces startup perf on arm again. https://codereview.chromium.org/2961413003/diff/1/base/memory/ref_counted.h File base/memory/ref_counted.h (right): https://codereview.chromium.org/2961413003/diff/1/base/memory/ref_counted.h#n... base/memory/ref_counted.h:180: ALWAYS_INLINE bool ReleaseImpl() const { Is the ALWAYS_INLINE needed? Seems like the compiler should be able to figure this out.
https://codereview.chromium.org/2961413003/diff/1/base/memory/ref_counted.h File base/memory/ref_counted.h (right): https://codereview.chromium.org/2961413003/diff/1/base/memory/ref_counted.h#n... base/memory/ref_counted.h:180: ALWAYS_INLINE bool ReleaseImpl() const { On 2017/06/30 16:19:06, (unused - use chromium) wrote: > Is the ALWAYS_INLINE needed? Seems like the compiler should be able to figure > this out. It should. But I think the macro also expresses our intent here: this really should be inlined without question, i.e. this is basically the macro approach discussed on the bug.
¯\_(ツ)_/¯ up to you On Fri, Jun 30, 2017 at 12:21 PM, <hans@chromium.org> wrote: > > https://codereview.chromium.org/2961413003/diff/1/base/ > memory/ref_counted.h > File base/memory/ref_counted.h (right): > > https://codereview.chromium.org/2961413003/diff/1/base/ > memory/ref_counted.h#newcode180 > base/memory/ref_counted.h:180: ALWAYS_INLINE bool ReleaseImpl() const { > On 2017/06/30 16:19:06, (unused - use chromium) wrote: > > Is the ALWAYS_INLINE needed? Seems like the compiler should be able to > figure > > this out. > > It should. > > But I think the macro also expresses our intent here: this really should > be inlined without question, i.e. this is basically the macro approach > discussed on the bug. > > https://codereview.chromium.org/2961413003/ > -- 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.
The CQ bit was unchecked by hans@chromium.org
The CQ bit was checked by hans@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
lgtm
The CQ bit was checked by hans@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/30 16:32:50, commit-bot: I haz the power wrote: > CQ is trying da patch. > > Follow status at: > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... lgtm thanks!
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1498840363493520, "parent_rev":
"c06f801f1d1d68122d17e300c5565fc9e8eda94c", "commit_rev":
"79eb7ebaabeb87fd76da56f35de0ab5776806a15"}
Message was sent while issue was closed.
Description was changed from ========== Outline RefCountedThreadSafeBase::AddRef and Release on non-X86 They were made inline in #481091, which caused a size drop on X86 and size increase on ARM. The code sequence on ARM is larger than on X86, and probably not worth inlining. BUG=728324,736015 ========== to ========== Outline RefCountedThreadSafeBase::AddRef and Release on non-X86 They were made inline in #481091, which caused a size drop on X86 and size increase on ARM. The code sequence on ARM is larger than on X86, and probably not worth inlining. BUG=728324,736015 Review-Url: https://codereview.chromium.org/2961413003 Cr-Commit-Position: refs/heads/master@{#483759} Committed: https://chromium.googlesource.com/chromium/src/+/79eb7ebaabeb87fd76da56f35de0... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/79eb7ebaabeb87fd76da56f35de0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
