Disable CFI on a few src/cc methods for perf reasons.

Disable CFI on a few src/cc methods for perf reasons.

While we have not observed CFI to slowdown real-world use cases,
there are a few blink_perf microbenchmarks, which are somewhat affected
by the change. This change removes CFI protection from the methods
which contribute the most to the slowdown.

Eventually, when a proposed optimization in Clang is implemented
(https://crbug.com/638056), these attributes would be possible to remove
from all/most of the methods.

BUG=638056, 634139
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/825ce48757eb1ec0428fe573262614635d25cb7f
Cr-Commit-Position: refs/heads/master@{#414916}

[krasin, 2016-08-19 00:24:08]

Description was changed from

==========
Disable CFI on a few src/cc methods for perf reasons.

While we have not observed CFI to slowdown real-world use cases,
there are a few blink_perf microbenchmarks, which a somewhat affected
by the change. This change removes CFI protection from the methods
which contribute the most to the slowdown.

Eventually, when a proposed optimization in Clang is implemented
(https://crbug.com/638056), these attributes would be possible to remove
from all/most of the methods.

BUG=638056,634139
==========

to

==========
Disable CFI on a few src/cc methods for perf reasons.

While we have not observed CFI to slowdown real-world use cases,
there are a few blink_perf microbenchmarks, which a somewhat affected
by the change. This change removes CFI protection from the methods
which contribute the most to the slowdown.

Eventually, when a proposed optimization in Clang is implemented
(https://crbug.com/638056), these attributes would be possible to remove
from all/most of the methods.

BUG=638056,634139
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[krasin, 2016-08-19 00:24:29]

Description was changed from

==========
Disable CFI on a few src/cc methods for perf reasons.

While we have not observed CFI to slowdown real-world use cases,
there are a few blink_perf microbenchmarks, which a somewhat affected
by the change. This change removes CFI protection from the methods
which contribute the most to the slowdown.

Eventually, when a proposed optimization in Clang is implemented
(https://crbug.com/638056), these attributes would be possible to remove
from all/most of the methods.

BUG=638056,634139
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Disable CFI on a few src/cc methods for perf reasons.

While we have not observed CFI to slowdown real-world use cases,
there are a few blink_perf microbenchmarks, which are somewhat affected
by the change. This change removes CFI protection from the methods
which contribute the most to the slowdown.

Eventually, when a proposed optimization in Clang is implemented
(https://crbug.com/638056), these attributes would be possible to remove
from all/most of the methods.

BUG=638056,634139
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[krasin, 2016-08-19 00:29:42]

krasin@google.com changed reviewers:
+ danakj@chromium.org

[krasin, 2016-08-19 00:29:42]

Hi Dana,

please, review this small CL. It turns of Control Flow Integrity checks on a few
compositor methods to avoid any perf regressions during the upcoming launch on
Linux x86-64. While it's not strictly required (there was no slowdown observed
on real-world use cases, the CFI checks are tiny), there're a few blink_perf
microbenchmarks which slowed down by 3%-5%, so I decided to be conservative and
exclude the methods with the most dynamic number of CFI checks from the first
launch.

Eventually, we plan to remove these attributes and reenable CFI protection, but
that requires more work on the compiler side to implement a few optimizations.

An alternative to these inline attributes is to add these methods into
tools/cfi/blacklist.txt similar to what I did for Skia:
https://codereview.chromium.org/2259143002/

Please, let me know your preferences.

[danakj, 2016-08-23 01:39:02]

How would someone adding a new method know whether it should get this attribute
or not? Like this seems like a snapshot of something but I have no idea as an
owner of cc/ how to maintain them. They cover a good chunk of the invalidation
critical path to the screen, but not all of it.

[krasin1, 2016-08-23 02:31:42]

On 2016/08/23 01:39:02, danakj wrote:
> How would someone adding a new method know whether it should get this
attribute
> or not? Like this seems like a snapshot of something but I have no idea as an
> owner of cc/ how to maintain them. They cover a good chunk of the invalidation
> critical path to the screen, but not all of it.

Just like with everything else: a new functionality is added, then Perf bots run
on the change and report if a substantial regression happened.
In case of CFI it's unlikely (only really hot functions with lots of
non-devirtualizable virtual calls do that), but possible.

If happened, I assume the person who added the new method would run a CPU
profiler and realize that it's the virtual call that's slow. Given these
DISABLE_CFI_PERF annotations around it would become clear the next step: add the
attribute to the function.

But again: I estimate the probability of this needed in practice as low, and
we're also working on a few compiler-side improvements which should lower the
CFI impact even further (so that most of these methods will get the attribute
removed). In particular, https://crbug.com/638056 should address about a half of
the cases. These would take time to implement, though.

Note: currently the very same methods are blacklisted in tools/cfi/blacklist.txt
and that has even worse visibility to the code owners:
https://cs.chromium.org/chromium/src/tools/cfi/blacklist.txt?q=tools/cfi/bl&s...
which is why I am going through this and adding the attributes to the code.

[krasin1, 2016-08-23 02:36:43]

Also, if we removed all the methods from the blacklist (added for perf reasons),
we'll only get ~3% slowdown on some blink microbenchmarks with one
microbenchmark regressed by ~5.5%: https://crbug.com/634139 (and that benchmark
is very noisy; a recent compiler update sped up it on Linux by 5% by itself; on
Mac that was even higher, like 15%)

To reiterate: I expect us to remove methods from the blacklist / remove the
attributes from the methods in the mid term, not adding more of them. The latter
is still possible in the short term, if it's realized that I missed something,
but that seems less and less likely with every day passed since Friday.

[krasin1, 2016-08-23 02:37:47]

On 2016/08/23 02:36:43, krasin1 wrote:
> Also, if we removed all the methods from the blacklist (added for perf
reasons),
By *all* methods I mean all: v8, cc, WebKit, skia. Most of the blacklisted
methods are actually in WebKit.

[krasin1, 2016-08-26 21:14:56]

On 2016/08/23 02:37:47, krasin1 wrote:
> On 2016/08/23 02:36:43, krasin1 wrote:
> > Also, if we removed all the methods from the blacklist (added for perf
> reasons),
> By *all* methods I mean all: v8, cc, WebKit, skia. Most of the blacklisted
> methods are actually in WebKit.

Unexpectedly, we have another good reason to move forward with the attributes:
since CFI blacklist is based on regular expressions, and these are not
particularly fast, the official build has been considerably slowed down due to
this: https://crbug.com/641343

While I will certainly speed this up on Clang side, CFI blacklist will still
burn some cycles, as there's a lookup for each method (and Chromium has a lot of
methods). The longer the blacklist, the slower the lookup. The attributes on the
other hand are applied to the specific methods, and don't require anything
complex.

Recently, I submitted similar CL to V8:
https://codereview.chromium.org/2258003002/ and that allowed to remove them from
the blacklist: https://codereview.chromium.org/2282753003/

It probably makes sense to do the same with cc methods.

[danakj, 2016-08-26 22:14:45]

LGTM

[danakj, 2016-08-26 22:16:00]

I guess it's better to have these in the code than in a file if anything, people
can notice that they exist. I'm a bit skeptical that we'd ever add a new one
which makes them feel a bit artificial. But then, this is just more ammunition
in the stop-using-virtuals-in-hot-paths bucket.

[danakj, 2016-08-26 22:18:12]

https://codereview.chromium.org/2257323002/diff/1/cc/base/contiguous_container.h
File cc/base/contiguous_container.h (right):

https://codereview.chromium.org/2257323002/diff/1/cc/base/contiguous_containe...
cc/base/contiguous_container.h:133: DISABLE_CFI_PERF
One question tho I didn't understand, why is this here? This isn't virtual so
I'd like to understand.

[krasin1, 2016-08-26 22:20:01]

I also hope that we only remove these attributes (one by one) instead of adding
more. Automatic devirtualization is already good and we have not yet pushed the
limits: https://storage.googleapis.com/cfi-stats/2016-08-15/devirt-methods.html

[krasin1, 2016-08-26 22:23:31]

On 2016/08/26 22:18:12, danakj wrote:
>
https://codereview.chromium.org/2257323002/diff/1/cc/base/contiguous_container.h
> File cc/base/contiguous_container.h (right):
> 
>
https://codereview.chromium.org/2257323002/diff/1/cc/base/contiguous_containe...
> cc/base/contiguous_container.h:133: DISABLE_CFI_PERF
> One question tho I didn't understand, why is this here? This isn't virtual so
> I'd like to understand.

CFI checks are on the call site side. It's blacklisted, because this is a hot
method that *calls* a virtual.

In particular, it calls a virtual destructor in a loop, and CFI emits a check
before every such call:

~ContiguousContainer() {
  for (auto& element : *this) {
    // MSVC incorrectly reports this variable as unused.
    (void)element;
    element.~BaseElementType();
  }
}

That should be possible to optimize, especially if the elements are (mostly) of
the same type, as the check results may be partially be reused between
iterations, see the speculative proposal for an optimization here:
https://bugs.chromium.org/p/chromium/issues/detail?id=638056#c1

[krasin1, 2016-08-26 22:27:42]

krasin@chromium.org changed reviewers:
+ krasin@chromium.org

[krasin1, 2016-08-26 22:27:42]

https://codereview.chromium.org/2257323002/diff/1/cc/base/contiguous_container.h
File cc/base/contiguous_container.h (right):

https://codereview.chromium.org/2257323002/diff/1/cc/base/contiguous_containe...
cc/base/contiguous_container.h:133: DISABLE_CFI_PERF
On 2016/08/26 22:18:12, danakj wrote:
> One question tho I didn't understand, why is this here? This isn't virtual so
> I'd like to understand.

Replied as a top-level comment (sorry)

[krasin1, 2016-08-26 22:27:52]

The CQ bit was checked by krasin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 22:28:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 22:33:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 22:33:18]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[danakj, 2016-08-26 22:41:41]

On Fri, Aug 26, 2016 at 3:23 PM, <krasin@chromium.org> wrote:

> On 2016/08/26 22:18:12, danakj wrote:
> >
> https://codereview.chromium.org/2257323002/diff/1/cc/base/
> contiguous_container.h
> > File cc/base/contiguous_container.h (right):
> >
> >
> https://codereview.chromium.org/2257323002/diff/1/cc/base/
> contiguous_container.h#newcode133
> > cc/base/contiguous_container.h:133: DISABLE_CFI_PERF
> > One question tho I didn't understand, why is this here? This isn't
> virtual so
> > I'd like to understand.
>
> CFI checks are on the call site side. It's blacklisted, because this is a
> hot
> method that *calls* a virtual.
>
> In particular, it calls a virtual destructor in a loop, and CFI emits a
> check
> before every such call:
>
> ~ContiguousContainer() {
> for (auto& element : *this) {
> // MSVC incorrectly reports this variable as unused.
> (void)element;
> element.~BaseElementType();
> }
> }
>
> That should be possible to optimize, especially if the elements are
> (mostly) of
> the same type, as the check results may be partially be reused between
> iterations, see the speculative proposal for an optimization here:
> https://bugs.chromium.org/p/chromium/issues/detail?id=638056#c1
>

Ah cool thanks for the explanation.


>
>
> https://codereview.chromium.org/2257323002/
>

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

[krasin1, 2016-08-26 22:46:51]

The CQ bit was checked by krasin@chromium.org

[krasin1, 2016-08-26 22:46:52]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2257323002/#ps20001
(title: "sync")

[commit-bot: I haz the power, 2016-08-26 22:47:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-27 11:01:27]

Description was changed from

==========
Disable CFI on a few src/cc methods for perf reasons.

While we have not observed CFI to slowdown real-world use cases,
there are a few blink_perf microbenchmarks, which are somewhat affected
by the change. This change removes CFI protection from the methods
which contribute the most to the slowdown.

Eventually, when a proposed optimization in Clang is implemented
(https://crbug.com/638056), these attributes would be possible to remove
from all/most of the methods.

BUG=638056,634139
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Disable CFI on a few src/cc methods for perf reasons.

While we have not observed CFI to slowdown real-world use cases,
there are a few blink_perf microbenchmarks, which are somewhat affected
by the change. This change removes CFI protection from the methods
which contribute the most to the slowdown.

Eventually, when a proposed optimization in Clang is implemented
(https://crbug.com/638056), these attributes would be possible to remove
from all/most of the methods.

BUG=638056,634139
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-08-27 11:01:28]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-27 11:04:20]

Description was changed from

==========
Disable CFI on a few src/cc methods for perf reasons.

While we have not observed CFI to slowdown real-world use cases,
there are a few blink_perf microbenchmarks, which are somewhat affected
by the change. This change removes CFI protection from the methods
which contribute the most to the slowdown.

Eventually, when a proposed optimization in Clang is implemented
(https://crbug.com/638056), these attributes would be possible to remove
from all/most of the methods.

BUG=638056,634139
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Disable CFI on a few src/cc methods for perf reasons.

While we have not observed CFI to slowdown real-world use cases,
there are a few blink_perf microbenchmarks, which are somewhat affected
by the change. This change removes CFI protection from the methods
which contribute the most to the slowdown.

Eventually, when a proposed optimization in Clang is implemented
(https://crbug.com/638056), these attributes would be possible to remove
from all/most of the methods.

BUG=638056,634139
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/825ce48757eb1ec0428fe573262614635d25cb7f
Cr-Commit-Position: refs/heads/master@{#414916}
==========

[commit-bot: I haz the power, 2016-08-27 11:04:21]

Patchset 2 (id:??) landed as
https://crrev.com/825ce48757eb1ec0428fe573262614635d25cb7f
Cr-Commit-Position: refs/heads/master@{#414916}