New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 641536 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocked on:
issue 670392

Blocking:
issue 641343



Sign in to add a comment

CFI blacklist is slow

Project Member Reported by krasin@chromium.org, Aug 26 2016

Issue description

As proved in https://crbug.com/641343, CFI blacklist slowdown significantly slows down the compilation (not linking).

For instance:

before:
53.42s	50m10.193s	51m3.613s		obj/net/net_unittests/quic_test_utils.o

after:
3m16.482s	1h21m13.999s	1h24m30.481s		obj/net/net_unittests/quic_test_utils.o
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/13f28a78f183528e9ba1747f02e4b012849f8dd9

commit 13f28a78f183528e9ba1747f02e4b012849f8dd9
Author: krasin <krasin@google.com>
Date: Sat Aug 27 23:45:22 2016

Remove most of src/cc functions from the CFI blacklist.

They are now excluded from CFI checks with DISABLE_CFI_PERF attributes,
see https://codereview.chromium.org/2257323002/

BUG= 641536 
TBR=kcc@chromium.org

Review-Url: https://codereview.chromium.org/2288523002
Cr-Commit-Position: refs/heads/master@{#414947}

[modify] https://crrev.com/13f28a78f183528e9ba1747f02e4b012849f8dd9/tools/cfi/blacklist.txt

Comment 2 by krasin@chromium.org, Aug 29 2016

Reproduced locally on a simple file with 256 nearly empty functions.

krasin@krasin2:~$ time clang++ -c many-funcs.cc -flto  -fsanitize=cfi-vcall -fvisibility=hidden 

real    0m0.099s
user    0m0.073s
sys     0m0.025s
krasin@krasin2:~$ time clang++ -c many-funcs.cc -flto  -fsanitize=cfi-vcall -fvisibility=hidden -fsanitize-blacklist=/usr/local/google/home/krasin/chr22/src/tools/cfi/blacklist.txt

real    0m2.154s
user    0m2.128s
sys     0m0.026s

The difference is 20x!

Comment 3 by krasin@chromium.org, Aug 29 2016

The profile shows that ~98% of the time is spent in regexp match, here is the stack trace:

#0  0x0000000001d10eec in lstep.isra ()
#1  0x0000000001d12c96 in llvm_regexec ()
#2  0x0000000001ce46fc in llvm::Regex::match (this=0x5bdf850, String=..., Matches=Matches@entry=0x0) at /usr/local/google/home/krasin/src/llvm.org/llvm/lib/Support/Regex.cpp:68
#3  0x00000000034a1dcd in match (Query=..., this=0x5bdf608) at /usr/local/google/home/krasin/src/llvm.org/llvm/lib/Support/SpecialCaseList.cpp:43
#4  llvm::SpecialCaseList::inSection (this=<optimized out>, Section=..., Query=..., Category=...) at /usr/local/google/home/krasin/src/llvm.org/llvm/lib/Support/SpecialCaseList.cpp:165
#5  0x0000000003455833 in clang::SanitizerBlacklist::isBlacklistedFunction (this=this@entry=0x5bd1410, FunctionName=...) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/lib/Basic/SanitizerBlacklist.cpp:33
#6  0x0000000001efd5f1 in clang::CodeGen::CodeGenModule::isInSanitizerBlacklist (this=0x5be6720, Fn=Fn@entry=0x5cbf088, Loc=..., Loc@entry=...) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/lib/CodeGen/CodeGenModule.cpp:1405
#7  0x0000000001ef428f in clang::CodeGen::CodeGenFunction::StartFunction (this=this@entry=0x7fffffff96e0, GD=..., GD@entry=..., RetTy=..., RetTy@entry=..., Fn=Fn@entry=0x5cbf088, FnInfo=..., Args=..., Loc=Loc@entry=..., StartLoc=StartLoc@entry=...) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/lib/CodeGen/CodeGenFunction.cpp:676
#8  0x0000000001ef7d0c in clang::CodeGen::CodeGenFunction::GenerateCode (this=this@entry=0x7fffffff96e0, GD=..., Fn=Fn@entry=0x5cbf088, FnInfo=...) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/lib/CodeGen/CodeGenFunction.cpp:1027
#9  0x0000000001f19bca in clang::CodeGen::CodeGenModule::EmitGlobalFunctionDefinition (this=this@entry=0x5be6720, GD=..., GV=0x5cbf088, GV@entry=0x0) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/lib/CodeGen/CodeGenModule.cpp:2944
#10 0x0000000001f2d620 in clang::CodeGen::CodeGenModule::EmitGlobalDefinition (this=this@entry=0x5be6720, GD=..., GV=GV@entry=0x0) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/lib/CodeGen/CodeGenModule.cpp:1813
#11 0x0000000001f2eae0 in clang::CodeGen::CodeGenModule::EmitGlobal (this=this@entry=0x5be6720, GD=...) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/lib/CodeGen/CodeGenModule.cpp:1631
#12 0x0000000001f2f217 in clang::CodeGen::CodeGenModule::EmitTopLevelDecl (this=0x5be6720, D=0x5cbfa58) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/lib/CodeGen/CodeGenModule.cpp:3802
#13 0x0000000001f2fbf7 in clang::CodeGen::CodeGenModule::EmitTopLevelDecl (this=<optimized out>, D=<optimized out>) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/lib/CodeGen/CodeGenModule.cpp:4003
#14 0x00000000024b0b74 in (anonymous namespace)::CodeGeneratorImpl::HandleTopLevelDecl (this=0x5be6160, DG=...) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/lib/CodeGen/ModuleBuilder.cpp:148
#15 0x00000000024aa88d in clang::BackendConsumer::HandleTopLevelDecl (this=0x5bdf350, D=...) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/lib/CodeGen/CodeGenAction.cpp:124
#16 0x0000000002833755 in clang::ParseAST (S=..., PrintStats=<optimized out>, SkipFunctionBodies=<optimized out>) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/lib/Parse/ParseAST.cpp:151
#17 0x00000000021b7539 in clang::ASTFrontendAction::ExecuteAction (this=this@entry=0x5bc2f00) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/lib/Frontend/FrontendAction.cpp:558
#18 0x00000000024ab5cd in clang::CodeGenAction::ExecuteAction (this=0x5bc2f00) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/lib/CodeGen/CodeGenAction.cpp:867
#19 0x00000000021b8366 in clang::FrontendAction::Execute (this=this@entry=0x5bc2f00) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/lib/Frontend/FrontendAction.cpp:458
#20 0x0000000002192168 in clang::CompilerInstance::ExecuteAction (this=this@entry=0x5bbe990, Act=...) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/lib/Frontend/CompilerInstance.cpp:871
#21 0x0000000002241bc2 in clang::ExecuteCompilerInvocation (Clang=Clang@entry=0x5bbe990) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:249
#22 0x0000000000a9ee58 in cc1_main (Argv=..., Argv0=Argv0@entry=0x7fffffffd348 "/usr/local/google/home/krasin/src/llvm.org/release-build/bin/clang-3.9", MainAddr=MainAddr@entry=0xa9ae20 <GetExecutablePath(char const*, bool)>) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/tools/driver/cc1_main.cpp:183
#23 0x0000000000a47b73 in ExecuteCC1Tool (Tool=..., argv=...) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/tools/driver/driver.cpp:299
#24 main (argc_=<optimized out>, argv_=<optimized out>) at /usr/local/google/home/krasin/src/llvm.org/llvm/tools/clang/tools/driver/driver.cpp:380

I will now look closely at SanitizerBlacklist::isBlacklistedFunction implementation.

Comment 4 by krasin@chromium.org, Aug 29 2016

There're no obvious low-hanging fruits there:

- the regexps are concatenated (so the regexp engine has an ability to build a more efficient finite automata; not sure if it does, though)
- the regexps are "compiled" (into an automata)

Seems like reducing the size of the blacklist is the way to go. I will still take a moment and try to understand if we can do anything inside Regexp engine or if we could switch the Regexp implementation to something faster.

Comment 5 by ajha@chromium.org, Aug 31 2016

Cc: ajha@chromium.org
Components: Build
Linux Build trigger is still really slow. 55.0.2845.0 compiled in 4 hrs 27 mins today.

krasin@: Could you please confirm if this issue is being tracked for Linux slowness.  

Comment 6 by krasin@chromium.org, Aug 31 2016

I confirm that I am still working on that. So far, ~30 minutes have been cut from the build time, and I expect to cut about the same with the upcoming CL to Blink.

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eb64c60f4789ab32bca921f63291ff8ccb35ab82

commit eb64c60f4789ab32bca921f63291ff8ccb35ab82
Author: krasin <krasin@chromium.org>
Date: Thu Sep 01 06:11:00 2016

Add DISABLE_CFI_PERF attribute on the methods with CFI checks disabled.

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 proposed optimizations in Clang are implemented
(https://crbug.com/638056, https://crbug.com/638064), these attributes
would be possible to remove from all/most of the methods.

This CL does not in fact change the list of disabled methods, it
just converts them from CFI blacklist entries into attributes.
This should increase visibility, and also serve as a guard against refactoring.

BUG= 641536 

Review-Url: https://codereview.chromium.org/2302733002
Cr-Commit-Position: refs/heads/master@{#415893}

[modify] https://crrev.com/eb64c60f4789ab32bca921f63291ff8ccb35ab82/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/eb64c60f4789ab32bca921f63291ff8ccb35ab82/third_party/WebKit/Source/core/layout/LayoutBox.h
[modify] https://crrev.com/eb64c60f4789ab32bca921f63291ff8ccb35ab82/tools/cfi/blacklist.txt

The CL above saved ~20 minutes on 'precise64 trunk'. The upcoming CL should save another 15-20 minutes: https://codereview.chromium.org/2304563003/
Cc: krasin@chromium.org thakis@chromium.org
 Issue 569738  has been merged into this issue.
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 6 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5e2204ad64fdcbc8efc4d1cad26a9da5672a39a8

commit 5e2204ad64fdcbc8efc4d1cad26a9da5672a39a8
Author: krasin <krasin@chromium.org>
Date: Tue Sep 06 21:00:19 2016

Add DISABLE_CFI_PERF attribute on the methods with CFI checks disabled.

Types converted in this CL:
LayoutBlock, LayoutBlockFlow, InlineFlowBox, InlineBox.

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 proposed optimizations in Clang are implemented
(https://crbug.com/638056, https://crbug.com/638064), these attributes
would be possible to remove from all/most of the methods.

This CL does not in fact change the list of disabled methods, it
just converts them from CFI blacklist entries into attributes.
This should increase visibility, and also serve as a guard against refactoring.

BUG= 641536 

Review-Url: https://codereview.chromium.org/2304563003
Cr-Commit-Position: refs/heads/master@{#416727}

[modify] https://crrev.com/5e2204ad64fdcbc8efc4d1cad26a9da5672a39a8/third_party/WebKit/Source/core/layout/LayoutBlock.cpp
[modify] https://crrev.com/5e2204ad64fdcbc8efc4d1cad26a9da5672a39a8/third_party/WebKit/Source/core/layout/LayoutBlock.h
[modify] https://crrev.com/5e2204ad64fdcbc8efc4d1cad26a9da5672a39a8/third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp
[modify] https://crrev.com/5e2204ad64fdcbc8efc4d1cad26a9da5672a39a8/third_party/WebKit/Source/core/layout/LayoutBlockFlow.h
[modify] https://crrev.com/5e2204ad64fdcbc8efc4d1cad26a9da5672a39a8/third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp
[modify] https://crrev.com/5e2204ad64fdcbc8efc4d1cad26a9da5672a39a8/third_party/WebKit/Source/core/layout/line/InlineBox.cpp
[modify] https://crrev.com/5e2204ad64fdcbc8efc4d1cad26a9da5672a39a8/third_party/WebKit/Source/core/layout/line/InlineBox.h
[modify] https://crrev.com/5e2204ad64fdcbc8efc4d1cad26a9da5672a39a8/third_party/WebKit/Source/core/layout/line/InlineFlowBox.h
[modify] https://crrev.com/5e2204ad64fdcbc8efc4d1cad26a9da5672a39a8/tools/cfi/blacklist.txt

Project Member

Comment 11 by bugdroid1@chromium.org, Sep 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7a5a65d12fdb71cba72df34881548cfaaf6b618c

commit 7a5a65d12fdb71cba72df34881548cfaaf6b618c
Author: krasin <krasin@chromium.org>
Date: Tue Sep 13 08:28:50 2016

Add DISABLE_CFI_PERF attribute on the methods with CFI checks disabled.

Types converted in this CL:
LayoutBlockModelObject, LayoutFlexibleObject, LayoutGrid, LayoutObject, LayoutObjectChildList.

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 proposed optimizations in Clang are implemented
(https://crbug.com/638056, https://crbug.com/638064), these attributes
would be possible to remove from all/most of the methods.

This CL does not in fact change the list of disabled methods, it
just converts them from CFI blacklist entries into attributes.
This should increase visibility, and also serve as a guard against refactoring.

BUG= 641536 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2332053002
Cr-Commit-Position: refs/heads/master@{#418196}

[modify] https://crrev.com/7a5a65d12fdb71cba72df34881548cfaaf6b618c/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
[modify] https://crrev.com/7a5a65d12fdb71cba72df34881548cfaaf6b618c/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h
[modify] https://crrev.com/7a5a65d12fdb71cba72df34881548cfaaf6b618c/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp
[modify] https://crrev.com/7a5a65d12fdb71cba72df34881548cfaaf6b618c/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
[modify] https://crrev.com/7a5a65d12fdb71cba72df34881548cfaaf6b618c/third_party/WebKit/Source/core/layout/LayoutObject.cpp
[modify] https://crrev.com/7a5a65d12fdb71cba72df34881548cfaaf6b618c/third_party/WebKit/Source/core/layout/LayoutObject.h
[modify] https://crrev.com/7a5a65d12fdb71cba72df34881548cfaaf6b618c/third_party/WebKit/Source/core/layout/LayoutObjectChildList.cpp
[modify] https://crrev.com/7a5a65d12fdb71cba72df34881548cfaaf6b618c/third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp
[modify] https://crrev.com/7a5a65d12fdb71cba72df34881548cfaaf6b618c/tools/cfi/blacklist.txt

Project Member

Comment 12 by bugdroid1@chromium.org, Sep 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e49ebf8c7b37e7d6d4868f93cd60d239d97a4abf

commit e49ebf8c7b37e7d6d4868f93cd60d239d97a4abf
Author: krasin <krasin@chromium.org>
Date: Tue Sep 13 20:49:49 2016

Convert a few more CFI blacklist entries into DISABLE_CFI_PERF attributes.

We're at 80% of the blacklist converted.

BUG= 641536 
TBR=eae@chromium.org

Review-Url: https://codereview.chromium.org/2336103003
Cr-Commit-Position: refs/heads/master@{#418361}

[modify] https://crrev.com/e49ebf8c7b37e7d6d4868f93cd60d239d97a4abf/third_party/WebKit/Source/core/css/ElementRuleCollector.cpp
[modify] https://crrev.com/e49ebf8c7b37e7d6d4868f93cd60d239d97a4abf/third_party/WebKit/Source/core/css/PropertySetCSSStyleDeclaration.cpp
[modify] https://crrev.com/e49ebf8c7b37e7d6d4868f93cd60d239d97a4abf/third_party/WebKit/Source/core/css/resolver/SharedStyleFinder.cpp
[modify] https://crrev.com/e49ebf8c7b37e7d6d4868f93cd60d239d97a4abf/third_party/WebKit/Source/core/dom/ContainerNode.cpp
[modify] https://crrev.com/e49ebf8c7b37e7d6d4868f93cd60d239d97a4abf/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/e49ebf8c7b37e7d6d4868f93cd60d239d97a4abf/third_party/WebKit/Source/core/dom/LayoutTreeBuilder.cpp
[modify] https://crrev.com/e49ebf8c7b37e7d6d4868f93cd60d239d97a4abf/third_party/WebKit/Source/core/dom/Node.cpp
[modify] https://crrev.com/e49ebf8c7b37e7d6d4868f93cd60d239d97a4abf/third_party/WebKit/Source/core/dom/Node.h
[modify] https://crrev.com/e49ebf8c7b37e7d6d4868f93cd60d239d97a4abf/third_party/WebKit/Source/core/events/EventTarget.h
[modify] https://crrev.com/e49ebf8c7b37e7d6d4868f93cd60d239d97a4abf/third_party/WebKit/Source/core/html/HTMLCollection.h
[modify] https://crrev.com/e49ebf8c7b37e7d6d4868f93cd60d239d97a4abf/third_party/WebKit/Source/core/html/HTMLElement.cpp
[modify] https://crrev.com/e49ebf8c7b37e7d6d4868f93cd60d239d97a4abf/third_party/WebKit/Source/core/layout/FloatingObjects.cpp
[modify] https://crrev.com/e49ebf8c7b37e7d6d4868f93cd60d239d97a4abf/tools/cfi/blacklist.txt

Project Member

Comment 13 by bugdroid1@chromium.org, Oct 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fb1595d32aa4bfba0cb83698ef73af61fff6038f

commit fb1595d32aa4bfba0cb83698ef73af61fff6038f
Author: krasin <krasin@chromium.org>
Date: Mon Oct 17 02:08:11 2016

Convert a few more CFI blacklist entries into DISABLE_CFI_PERF attributes.

We're at ~90% of the blacklist converted, and the rest is hard:

* some of the entries are disabled because of the known bugs in Chromium,
* skia upstream considered to be not a suitable place for Chrome-specific macros,
* a few entries point to more than one function (like traceImpl) and a regexp is justified.

BUG= 641536 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2419663009
Cr-Commit-Position: refs/heads/master@{#425607}

[modify] https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f/third_party/WebKit/Source/core/css/PropertySetCSSStyleDeclaration.cpp
[modify] https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp
[modify] https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f/third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp
[modify] https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f/third_party/WebKit/Source/core/page/FrameTree.cpp
[modify] https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f/third_party/WebKit/Source/core/page/PageAnimator.cpp
[modify] https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f/third_party/WebKit/Source/core/paint/BlockPainter.cpp
[modify] https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f/third_party/WebKit/Source/core/paint/BoxClipper.cpp
[modify] https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f/third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp
[modify] https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f/third_party/WebKit/Source/platform/PODIntervalTree.h
[modify] https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f/third_party/WebKit/Source/platform/graphics/ContiguousContainer.h
[modify] https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f/third_party/WebKit/Source/platform/graphics/DEPS
[modify] https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f/third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp
[modify] https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f/third_party/WebKit/Source/platform/graphics/paint/DrawingDisplayItem.h
[modify] https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f/third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.cpp
[modify] https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f/third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
[modify] https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f/third_party/WebKit/Source/web/ChromeClientImpl.cpp
[modify] https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f/tools/cfi/blacklist.txt

Project Member

Comment 14 by bugdroid1@chromium.org, Oct 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ee27526940dbf16abf1c3f3ee4513e447bbd12b1

commit ee27526940dbf16abf1c3f3ee4513e447bbd12b1
Author: krasin <krasin@chromium.org>
Date: Mon Oct 17 02:26:52 2016

Revert of Convert a few more CFI blacklist entries into DISABLE_CFI_PERF attributes. (patchset #1 id:1 of https://codereview.chromium.org/2419663009/ )

Reason for revert:
Possibly broke Chrome OS:
https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20ChromeOS/builds/24655

Original issue's description:
> Convert a few more CFI blacklist entries into DISABLE_CFI_PERF attributes.
>
> We're at ~90% of the blacklist converted, and the rest is hard:
>
> * some of the entries are disabled because of the known bugs in Chromium,
> * skia upstream considered to be not a suitable place for Chrome-specific macros,
> * a few entries point to more than one function (like traceImpl) and a regexp is justified.
>
> BUG= 641536 
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
>
> Committed: https://crrev.com/fb1595d32aa4bfba0cb83698ef73af61fff6038f
> Cr-Commit-Position: refs/heads/master@{#425607}

TBR=tkent@chromium.org,eae@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 641536 

Review-Url: https://codereview.chromium.org/2422973002
Cr-Commit-Position: refs/heads/master@{#425610}

[modify] https://crrev.com/ee27526940dbf16abf1c3f3ee4513e447bbd12b1/third_party/WebKit/Source/core/css/PropertySetCSSStyleDeclaration.cpp
[modify] https://crrev.com/ee27526940dbf16abf1c3f3ee4513e447bbd12b1/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp
[modify] https://crrev.com/ee27526940dbf16abf1c3f3ee4513e447bbd12b1/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/ee27526940dbf16abf1c3f3ee4513e447bbd12b1/third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp
[modify] https://crrev.com/ee27526940dbf16abf1c3f3ee4513e447bbd12b1/third_party/WebKit/Source/core/page/FrameTree.cpp
[modify] https://crrev.com/ee27526940dbf16abf1c3f3ee4513e447bbd12b1/third_party/WebKit/Source/core/page/PageAnimator.cpp
[modify] https://crrev.com/ee27526940dbf16abf1c3f3ee4513e447bbd12b1/third_party/WebKit/Source/core/paint/BlockPainter.cpp
[modify] https://crrev.com/ee27526940dbf16abf1c3f3ee4513e447bbd12b1/third_party/WebKit/Source/core/paint/BoxClipper.cpp
[modify] https://crrev.com/ee27526940dbf16abf1c3f3ee4513e447bbd12b1/third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp
[modify] https://crrev.com/ee27526940dbf16abf1c3f3ee4513e447bbd12b1/third_party/WebKit/Source/platform/PODIntervalTree.h
[modify] https://crrev.com/ee27526940dbf16abf1c3f3ee4513e447bbd12b1/third_party/WebKit/Source/platform/graphics/ContiguousContainer.h
[modify] https://crrev.com/ee27526940dbf16abf1c3f3ee4513e447bbd12b1/third_party/WebKit/Source/platform/graphics/DEPS
[modify] https://crrev.com/ee27526940dbf16abf1c3f3ee4513e447bbd12b1/third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp
[modify] https://crrev.com/ee27526940dbf16abf1c3f3ee4513e447bbd12b1/third_party/WebKit/Source/platform/graphics/paint/DrawingDisplayItem.h
[modify] https://crrev.com/ee27526940dbf16abf1c3f3ee4513e447bbd12b1/third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.cpp
[modify] https://crrev.com/ee27526940dbf16abf1c3f3ee4513e447bbd12b1/third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
[modify] https://crrev.com/ee27526940dbf16abf1c3f3ee4513e447bbd12b1/third_party/WebKit/Source/web/ChromeClientImpl.cpp
[modify] https://crrev.com/ee27526940dbf16abf1c3f3ee4513e447bbd12b1/tools/cfi/blacklist.txt

We still see Chrome Linux builds are still taking 4 to 5 hours to build. Please look into this ASAP. This is impacting the productivity of 30 testers because half of the day is gone by the time builds show up.
Will work on this tomorrow. For the record, it's unlikely due to the CFI blacklist slowness, as the CFI blacklist is almost gone (modulo Skia and modulo the reverted CL, see #14). It also used to be almost 1 hour faster: https://uberchromegw.corp.google.com/i/official.desktop/builders/precise64/builds/1100

The blacklist didn't grow since that time; something else is regressed.

I totally agree that >4 hours builds are intolerable, and will try to understand what is the current reason for this slowness.

I also expect to reland #14 to make sure everything that can be done about the CFI blacklist is actually done.

Thank you for the report.

Comment 17 by mmoss@chromium.org, Nov 17 2016

It looks like something did recently add about 20 minutes between https://uberchromegw.corp.google.com/i/official.desktop/builders/precise64/builds/1238 and https://uberchromegw.corp.google.com/i/official.desktop/builders/precise64/builds/1240.

Compile step was about 3:30 before, now consistently more like 3:50. That doesn't explain the hour or more jump since M55, but it's something.
There have not been an Clang rolls in that period, so we can figure out the compiler/linker regressions for this particular 20 minutes bump.
The latest Clang version update was:

commit 5e529e0684b9ea3064fe944cee6635824dc97a67
Author: thakis <thakis@chromium.org>
Date:   Mon Oct 24 20:48:48 2016 -0700

    Roll clang 283753:284979.
    
    Ran `tools/clang/scripts/upload_revision.py 284979`.
    
    BUG= 656667 
    
    Review-Url: https://codereview.chromium.org/2443963002
    Cr-Commit-Position: refs/heads/master@{#427269}

A CL to reland #14 is created: https://codereview.chromium.org/2517653002/
It has 80% of the stuff from #14, and once it's submitted I will take care of the rest. After that, I plan to take a very close look at ninja build times on the bots.
Project Member

Comment 21 by bugdroid1@chromium.org, Nov 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/da258e1713662615e9a3aab57501105b33883d0b

commit da258e1713662615e9a3aab57501105b33883d0b
Author: krasin <krasin@chromium.org>
Date: Fri Nov 18 22:43:35 2016

Convert a few more CFI blacklist entries into DISABLE_CFI_PERF attributes.

This is an attempt to re-land a part of https://codereview.chromium.org/2419663009

BUG= 641536 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
TBR=tkent

Review-Url: https://codereview.chromium.org/2517653002
Cr-Commit-Position: refs/heads/master@{#433320}

[modify] https://crrev.com/da258e1713662615e9a3aab57501105b33883d0b/third_party/WebKit/Source/core/css/PropertySetCSSStyleDeclaration.cpp
[modify] https://crrev.com/da258e1713662615e9a3aab57501105b33883d0b/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp
[modify] https://crrev.com/da258e1713662615e9a3aab57501105b33883d0b/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/da258e1713662615e9a3aab57501105b33883d0b/third_party/WebKit/Source/core/page/FrameTree.cpp
[modify] https://crrev.com/da258e1713662615e9a3aab57501105b33883d0b/third_party/WebKit/Source/core/page/PageAnimator.cpp
[modify] https://crrev.com/da258e1713662615e9a3aab57501105b33883d0b/third_party/WebKit/Source/core/paint/BlockPainter.cpp
[modify] https://crrev.com/da258e1713662615e9a3aab57501105b33883d0b/third_party/WebKit/Source/core/paint/BoxClipper.cpp
[modify] https://crrev.com/da258e1713662615e9a3aab57501105b33883d0b/third_party/WebKit/Source/core/paint/ObjectPaintInvalidator.cpp
[modify] https://crrev.com/da258e1713662615e9a3aab57501105b33883d0b/third_party/WebKit/Source/platform/graphics/ContiguousContainer.h
[modify] https://crrev.com/da258e1713662615e9a3aab57501105b33883d0b/third_party/WebKit/Source/platform/graphics/DEPS
[modify] https://crrev.com/da258e1713662615e9a3aab57501105b33883d0b/third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp
[modify] https://crrev.com/da258e1713662615e9a3aab57501105b33883d0b/third_party/WebKit/Source/platform/graphics/paint/DrawingDisplayItem.h
[modify] https://crrev.com/da258e1713662615e9a3aab57501105b33883d0b/third_party/WebKit/Source/platform/graphics/paint/PaintArtifact.cpp
[modify] https://crrev.com/da258e1713662615e9a3aab57501105b33883d0b/third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
[modify] https://crrev.com/da258e1713662615e9a3aab57501105b33883d0b/tools/cfi/blacklist.txt

I have relanded most of the CL from #14. Now, I will wait a bit to confirm that no bots are broken. Then (likely, on Monday) there will be a small follow up CL with a few more entries removed. In parallel, I will profile the build.
The bots are green, and I have made the next step: relanding the single line that broke ChromeOS in #14: https://codereview.chromium.org/2517993003 (with a fix).

The release bots have completed their builds a few times after landing #21, and there are no real gains there. I plan to analyze .ninja_log files from the bots today and post the results here.
Project Member

Comment 24 by bugdroid1@chromium.org, Nov 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/565ff1e9c4a5bcda8db37bdd8365a7eee9f3c029

commit 565ff1e9c4a5bcda8db37bdd8365a7eee9f3c029
Author: krasin <krasin@chromium.org>
Date: Mon Nov 21 23:14:11 2016

Convert one more CFI blacklist entry into DISABLE_CFI_PERF attribute.

This one is tricky as it broke Chrome OS build last time. The line
now moved below the template parameter, and that should work.

The primary problem here is that Chrome OS lacks a trybot here,
so it's impossible to know in advance, if the change is going to break it.

BUG= 641536 
TBR=eae

Review-Url: https://codereview.chromium.org/2517993003
Cr-Commit-Position: refs/heads/master@{#433684}

[modify] https://crrev.com/565ff1e9c4a5bcda8db37bdd8365a7eee9f3c029/third_party/WebKit/Source/platform/PODIntervalTree.h
[modify] https://crrev.com/565ff1e9c4a5bcda8db37bdd8365a7eee9f3c029/tools/cfi/blacklist.txt

End of day update: I've got my hands on the ninja log files from the 'precise64' official buildbot, but I am still thinking how to properly analyze them, as there are no really obvious hints there.
I have completed the analysis of the ninja logs.

First, up to Nov, 9th we had very slow builds of Chrome 54, because my CFI blacklist fixes have not been cherry-picked to that release branch.

Second, I have drawn a 'Total compile time' graph that shows the impact of the work in this CL:
https://docs.google.com/spreadsheets/d/1BBdt0vvKRoTOQKegucnRk9QtLUU9wg7-J-A9IGs5LKM/edit#gid=761222487

Initially, we had fast M53 (before CFI was enabled), very slow M54 (before the fixes in this CL) and somewhat faster M55. Now, we have M55, M56 and M57 which are much closer to each other.

The remaining CFI compile-time penalty is still big: 37%.

The 'Total link time' graph shows that LTO+CFI link times are 17% slower than LTO link times. But we clearly see that there were not regressions from LLVM side, which is good.

That gives two insights:

1. I need to take a very close look at the compile times again. May be it's the remainder of the CFI blacklist, but may be something else is slow.
2. Link times are slower, but they probably can wait for ThinLTO being ready:  https://crbug.com/660216 , where the link times will be cut by 4x-5x.

Tomorrow I will spend the whole day on profiling (and possibly fixing) the compile times.
Profiling demonstrated the source of the blacklist slowness: for some of the compile units the check is called >25000 due to lots of template functions like:

IPC::MessageT<ChildProcessMsg_Shutdown_Meta, std::tuple<>, void>::MessageT(IPC::Routing)
IPC::MessageT<AppCacheMsg_ContentBlocked_Meta, std::tuple<int, GURL>, void>::Read(IPC::Message const*, std::tuple<int, GURL>*)
AccessibilityHostMsg_EventParams::AccessibilityHostMsg_EventParams()
IPC::ParamTraits<tracked_objects::ThreadData::Status>::GetSize(base::PickleSizer*, tracked_objects::ThreadData::Status const&)
std::_Tuple_impl<2ul, gpu::GPUInfo>::_Tuple_impl()
std::_Head_base<3ul, GURL, false>::~_Head_base()
std::basic_string<unsigned short, base::string16_char_traits, std::allocator<unsigned short> >* std::__addressof<std::basic_string<unsigned short, base::string16_char_traits, std::allocator<unsigned short> > >(std::basic_string<unsigned short, base::string16_char_traits, std::allocator<unsigned short> >&)

and so on.

I have put some thought how to implement a fast path that won't trigger the full regexp for each of these functions. My prototype filters 99% of inputs while spending 0.15 seconds (including parsing the rules, the inputs and preparing the data structures). That's very early results, and there is still a chance that I made a trivial mistake somewhere, but the general idea is solid:

1. All our rules in the blacklist look like "*allocate*Backing*" or "*EtwTraceProvider*" or the like. Most of the time there are enough regular characters in a row, so the rules like "*a*b*c*" are unseen. They should be supported, but it's probably okay to not try hard to make the fast path working for them (I don't know an idea for that anyway)

2. Let's break uninterrupted sequences from the rules into triples, like:

*allocate*Backing* -> all, llo, loc, oca, cat, ate, Bac, ack, cki, kin, ing

3. In order for an input to satisfy a rule, it needs to have all these triples somewhere inside. If it does not have a single of them => the rule is not satisfied. If we can check this property fast enough, we can filter out 99% of the inputs (as proved by the prototype)

4. To check that quickly, I first create a map [triple -> list of rules], then delete triples with more than 4 rules attached (as a speed optimization), and then compute the number of triples per rule that must be matched by this map.
I store these numbers in Counts array.

5. At inference time, I initialize a new CurCounts array (with zeros), go through each triple in the input and count rules according to the map. If at some point CurCounts[i] >= Counts[i], I declare the fast path defeated and proceed to the regular expression matching. If all CurCounts[i] < Counts[i] after all triples are processed, we know that none of the rules could match the input and we skip it.

My prototype is in Go, and I plan to properly implement it in LLVM codebase after the holiday break, but I am optimistic about making the blacklist much faster.
maybe the same approach could work for https://llvm.org/bugs/show_bug.cgi?id=30656 too

Comment 29 by p...@chromium.org, Nov 24 2016

That's an interesting idea, although of course I recognised it (https://swtch.com/~rsc/regexp/regexp4.html).

Have you also confirmed that the time spent demangling function names is insignificant?
@thakis: maybe

@pcc: I don't demangle functions, so the amount of time spent on this is zero.

I also have not claimed it's mine and I borrowed from a different (older) source than https://swtch.com/~rsc/regexp/regexp4.html. In particular, the very same idea was used in "Fast SMB Search" engine written by Alexander Vodomerov (alexvod@google.com) while he was at Moscow State University (~2005, I believe). And most likely someone used the idea in 70s.

Comment 31 by p...@chromium.org, Nov 24 2016

> @pcc: I don't demangle functions, so the amount of time spent on this is zero.

I am referring to the time that Clang spends demangling function names, not your prototype.

Comment 32 by krasin@google.com, Nov 24 2016

Clang does not demangle them either. Matching is against mangled names.

Comment 33 by p...@chromium.org, Nov 24 2016

I see, thanks.
@pcc -- thank you for the link to rsc' blog post. It was an entertaining logical Friday read. I like this piece:

"""
Neither n-grams nor their application to pattern matching is new. Shannon used n-grams in his seminal 1948 paper “A Mathematical Theory of Communication” [1] to analyze the information content of English text. Even then, n-grams were old hat. Shannon cites Pratt's 1939 book Secret and Urgent [2], and one imagines that Shannon used n-gram analysis during his wartime cryptography work.
"""

My guess about when the idea was first used was very wrong. :)
End of day update: I have a work-in-progress CL that works, speeds up things, but can and will be improved: https://reviews.llvm.org/D27188

At this point, it's not up for a review. May be it will reach reviewable state by the end of tomorrow.
https://reviews.llvm.org/D27188 is sent for a review. According to the perf profile of clang compiling one of the slowest files (content/common/common/content_message_generator.cc) with CFI, dealing with blacklist takes around 1% of the time. That's reasonable.
Nice :-)
The change is submitted as r288303. I plan to make a follow up to support escaping, like UBSanVptr blacklist uses: https://cs.chromium.org/chromium/src/tools/ubsan/vptr_blacklist.txt?q=vptr_&sq=package:chromium&l=119

src:*/third_party/libc\+\+abi/trunk/src/private_typeinfo.cpp

but for the CFI purposes it's already good enough and the next Clang roll should speed up the release builds for the branches which will use it. 
Based on the (still ongoing) 'CFI Linux ToT' run, the change has reduced the amount of wall time spent on compilation (not counting link time) from 67 to 60 minutes. The official bots (M57) spend 87 minutes on compilation. It's not unreasonable to project that the gains will be proportional (87 -> 78, or just 9 minutes).

While that will close the current issue, it seems that I need to put more effort on speeding up the linking. It takes considerable time based on the graph: https://screenshot.googleplex.com/ywcU88b33b3.png

Blockedon: 670392
Today, I have implemented support for escaped symbols which should somewhat help UBSan builders, since ubsan/vptr_blacklist.txt has a few '\+' entries.

I am also looking into Clang perf profile in a hope I could find sometimes else to optimize. llvm::PMDataManager::findAnalysisPass is currently the best candidate for that, as it unnecessarily eats ~3% of the time (the number is not strict). It seems to be too slow due to using DenseMap.
Is this done?

Comment 43 Deleted

yes, the CFI blacklist is no longer slow. Regarding to the speed of release build, the improvement there would be switching to ThinLTO as tracked by  https://crbug.com/660216 . At the moment, ThinLTO is very unstable, not much faster due to a regression in LLD, and does not support nor CFI, nor whole program devirtualization. But multiple people are working to address these issue, so they should be resolved earlier or later.

Sign in to add a comment