Issue metadata
Sign in to add a comment
|
11.2%-21.5% regression in blink_perf.events at 384207:384225 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Apr 1 2016
=== Auto-CCing suspected CL author kotenkov@yandex-team.ru === Hi kotenkov@yandex-team.ru, the bisect results pointed to your CL below as possibly causing a regression. Please have a look at this info and see whether your CL be related. ===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : Replace all occurrences of RELEASE_ASSERT in wtf with CHECK. Author : kotenkov Commit description: BUG= 596760 Review URL: https://codereview.chromium.org/1840163002 Cr-Commit-Position: refs/heads/master@{#384213} Commit : d181f524c11e0d456bc48261334beda61e6f852c Date : Thu Mar 31 07:41:16 2016 ===== TESTED REVISIONS ===== Revision Mean Value Std. Dev. Num Values Good? chromium@384208 5448.408739 17.731483 5 good chromium@384212 5604.739663 55.365819 5 good chromium@384213 5080.612039 24.786403 5 bad <- chromium@384214 5059.898498 70.839571 5 bad chromium@384216 5080.120092 49.000575 5 bad Bisect job ran on: android_nexus6_perf_bisect Bug ID: 599867 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --also-run-disabled-tests blink_perf.events Test Metric: EventsDispatchingInShadowTrees/EventsDispatchingInShadowTrees Relative Change: 6.76% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus6_perf_bisect/builds/2071 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9016575274391819744 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=599867 | O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq | X | for more information addressing perf regression bugs. For feedback, | / \ | file a bug with component Tests>AutoBisect. Thank you!
,
Apr 1 2016
Hi haraken. It seems that RELEASE_ASSERT -> CHECK conversion brings some regressions. Would you please advise me on how to better deal with this issue? Should we revert the patch at once, or should I just investigate the issue and fix it? Also, I would appreciate an advice on how to debug this issue since it seems to be android specific and I don't have an android device on hand.
,
Apr 1 2016
I can't answer all the questions, but you can use performance try bots to run the test on Android: https://www.chromium.org/developers/telemetry/performance-try-bots
,
Apr 1 2016
Hmm, this looks like a real regression. Maybe do we need to add LIKELY/UNLIKELY to the CHECK implementation? Blink's RELEASE_ASSERT had LIKELY/UNLIKELY. I'd recommend you try either of the following options: - Revert the CL and confirm that the CL is a real culprit; or - Add LIKELY/UNLIKELY to the CHECK implementation and see if it fixes the regression.
,
Apr 3 2016
Let's revert the culprit CL anyway. Then, identify which CHECK caused it by perf try bot.
,
Apr 3 2016
I've investigated this issue a bit on the weekend. The CL really caused the regressions, here is the blink_perf.events for the revert (https://codereview.chromium.org/1845923005/): http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-04-01_16-51-14 I've run some perf try jobs to determine the culprit. This one is with Vector.h reverted to RELEASE_ASSERTS: http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-04-02_13-01-39 (https://codereview.chromium.org/1853933002) This one is with StringImpl.{cpp,h} and WTFString.cpp reverted to RELEASE_ASSERTS: http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-04-02_13-11-35 (https://codereview.chromium.org/1846723008) This one is with everything except Vector.h and strings reverted to RELEASE_ASSERTS: http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-04-03_01-46-39 (https://codereview.chromium.org/1855763002) This one is with added __builtin_expect: http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-04-02_14-50-41 (https://codereview.chromium.org/1857583002/) It seems that the conversion of Vector.h accounts for most of the regression and the strings contributed a bit too. Adding UNLIKELY/LIKELY doesn't seem to affect the things much. After the revert is merged, I'll make a CL with conversion of non-bottleneck files. As of the root of the issue, I don't yet understand why CHECKS are so much slower than RELEASE_ASSERTS. UNLIKELY doesn't seem to be the reason.
,
Apr 3 2016
> As of the root of the issue, I don't yet understand why CHECKS are so much slower than RELEASE_ASSERTS. UNLIKELY doesn't seem to be the reason. I guess CHECK produces more instructions than RELEASE_ASSERT. Of course it consumes additional time to run, and larger code size increases cache miss rate of CPU instruction cache.
,
Apr 4 2016
,
Apr 4 2016
Can we simply revert the change first? Now over 100 alerts were raised on the perf dashboard, and it slightly make the perf regression triage process harder.
,
Apr 4 2016
Yes, let's revert it.
,
Apr 4 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7 commit 6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7 Author: toyoshim <toyoshim@chromium.org> Date: Mon Apr 04 12:25:50 2016 Revert of Replace all occurrences of RELEASE_ASSERT in wtf with CHECK. (patchset #4 id:60001 of https://codereview.chromium.org/1840163002/ ) Reason for revert: Caused performance regression. Original issue's description: > Replace all occurrences of RELEASE_ASSERT in wtf with CHECK. > > BUG= 596760 > > Committed: https://crrev.com/d181f524c11e0d456bc48261334beda61e6f852c > Cr-Commit-Position: refs/heads/master@{#384213} TBR=haraken@chromium.org,yutak@chromium.org,kotenkov@yandex-team.ru # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 596760 , 599867 Review URL: https://codereview.chromium.org/1859513002 Cr-Commit-Position: refs/heads/master@{#384889} [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/ArrayBuffer.h [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/Deque.h [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/Functional.h [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/HashTable.h [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/Optional.h [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/PageAllocator.cpp [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/PartitionAlloc.cpp [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/PartitionAlloc.h [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/PartitionAllocator.h [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/Partitions.cpp [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/TerminatedArrayBuilder.h [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/TypedArrayBase.h [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/Vector.h [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/WTF.cpp [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/dtoa/utils.h [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/text/AtomicString.cpp [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/text/CString.cpp [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/text/StringConcatenate.cpp [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/text/StringConcatenate.h [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/text/StringImpl.cpp [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/text/StringImpl.h [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/text/TextCodecUTF16.cpp [modify] https://crrev.com/6f9ac9d3e4eaaf5185d6c9067277119bfedc72d7/third_party/WebKit/Source/wtf/text/WTFString.cpp
,
Apr 5 2016
The revert fixed regressions.
,
Apr 5 2016
What happens if you use a version of CHECK that does not take stream parameters?
,
Apr 6 2016
> What happens if you use a version of CHECK that does not take stream parameters?
What version do you have in mind?
I've looked at
#define CHECK(condition) \
LAZY_STREAM(logging::LogMessage(__FILE__, __LINE__, #condition).stream(), \
!(condition))
because it seems that this is the version that is used on android.
I've written a small program to compare the to macros: http://pastebin.com/uWay113b. On my mac the CHECK is about 15% slower.
Here is the code of the two functions after preprocessor:
__attribute__((noinline))
void test_check(bool condition) {
!(!(condition)) ? (void) 0 : ::logging::LogMessageVoidify() & (logging::LogMessage("../../regression/regression.cc", 7, "condition").stream());
}
__attribute__((noinline))
void test_assert(bool condition) {
(__builtin_expect((!(condition)), 0) ? (__builtin_trap()) : (void)0);
}
And here is their assembly on my mac:
__Z10test_checkb:
0000000100001400 pushq %rbp
0000000100001401 movq %rsp, %rbp
0000000100001404 pushq %r14
0000000100001406 pushq %rbx
0000000100001407 subq $0x130, %rsp ## imm = 0x130
000000010000140e movq 0x31c53(%rip), %rbx
0000000100001415 movq (%rbx), %rbx
0000000100001418 movq %rbx, -0x18(%rbp)
000000010000141c testb %dil, %dil
000000010000141f jne 0x10000144b
0000000100001421 leaq 0x27621(%rip), %rsi
0000000100001428 leaq 0x27639(%rip), %rcx
000000010000142f leaq -0x140(%rbp), %r14
0000000100001436 movl $0x7, %edx
000000010000143b movq %r14, %rdi
000000010000143e callq 0x1000026c0
0000000100001443 movq %r14, %rdi
0000000100001446 callq 0x1000026f0
000000010000144b cmpq -0x18(%rbp), %rbx
000000010000144f jne 0x10000145d
0000000100001451 addq $0x130, %rsp ## imm = 0x130
0000000100001458 popq %rbx
0000000100001459 popq %r14
000000010000145b popq %rbp
000000010000145c retq
000000010000145d callq 0x100027356
0000000100001462 nopw %cs:(%rax,%rax)
__Z11test_assertb:
0000000100001470 pushq %rbp
0000000100001471 movq %rsp, %rbp
0000000100001474 subq $0x10, %rsp
0000000100001478 movq 0x31be9(%rip), %rax
000000010000147f movq (%rax), %rax
0000000100001482 movq %rax, -0x8(%rbp)
0000000100001486 testb %dil, %dil
0000000100001489 je 0x100001497
000000010000148b cmpq -0x8(%rbp), %rax
000000010000148f jne 0x100001499
0000000100001491 addq $0x10, %rsp
0000000100001495 popq %rbp
0000000100001496 retq
0000000100001497 ud2
0000000100001499 callq 0x100027356
000000010000149e nop
The CHECK is just doing much more things.
,
Apr 6 2016
I finally understood what version you've meant. Here are perf results: http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-04-06_09-35-37 (https://codereview.chromium.org/1865063003). It seems that there is no regression if we use CHECK without error messages. The checks on android use the heavy implementation since https://codereview.chromium.org/336413005 . If the messages are still needed in other places, we can check for BLINK_IMPLEMENTATION and use the light version in Blink.
,
Apr 8 2016
Thanks, then maybe we should remove error messages from checks. It's reduce the binary size we ship as well. What do y'all think? Sounds good to remove messages from CHECK()?
,
Apr 8 2016
Consider: 1. removing messages, but leaving the messages from CHECK in for DCHECK_IS_ON() builds, or 2. having a FAST_CHECK (bikeshed here) for use in performance sensitive places, or 3. 2, but with messages included when DCHECK_IS_ON()
,
Apr 8 2016
Er, let me try to understand. Your patch just changed the official build on android? It looks like we dont have messages in official builds already, except android for some reason. Why would we want android to have messages in the official build? Removing those seems like a progression for android to me. https://chromium.googlesource.com/chromium/src/+/84921ef6016d6df808a3491eeb0279e9abf86637 : > [Android] Spill out CHECK error message in official build > > In M35 release, we had a lot of user feedbacks of Chrome native crash. > We had great difficulty to investigate these native crashes > because user feedbacks don't have minidumps. > > By keeping the CHECK error message in logcat, it'd be a lot easier > to investigate these user reports of Chrome crashes. > > The binary size increase: ~0.5% (188k out of 34.4M). Binary and perf change, this change seems undesirable. What about the regressions on mac/windows in https://chromeperf.appspot.com/group_report?bug_id=599867 are those not related then?
,
Apr 8 2016
> Your patch just changed the official build on android? > It looks like we dont have messages in official builds already, except android for some reason. Both are correct. > Why would we want android to have messages in the official build? Some reasons are in the CL that I mentioned earlier, I'm not sure if they are still applicable: > > Why are clients unable to send minidumps? > There are a lot of reasons, network, out of memory, we limit the number of minidumps for saving bandwidth, etc. > A user may not be on WiFi for days, I don't know how to fix it. > What about the regressions on mac/windows in https://chromeperf.appspot.com/group_report?bug_id=599867 are those not related then? Let's see. I've started the corresponding perf try jobs, will post an update when they are done. (https://codereview.chromium.org/1866133005 https://codereview.chromium.org/1871823002)
,
Apr 8 2016
Here are the results of the win test: http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-04-08_01-54-43 It seems there are regressions in blink_perf.layout:large-table-with-collapsed-borders-and-colspans-wider-than-table and blink_perf.layout:floats_10_1000. I'll investigate them further. As for the mac_hdd try job, results are here: https://build.chromium.org/p/tryserver.chromium.perf/builders/mac_hdd_perf_bisect/builds/471 , but there is no HTML results link and I have no access to other results (I guess, for some unknown reason it's Google-only).
,
Apr 8 2016
@danakj: See discussion on https://codereview.chromium.org/336413005 . If this is causing more problems now, we might want to revisit that decision.
,
Apr 11 2016
I'm curious what if (on android?) we made CHECK print out the file/line but not deal with the stream parameters. Could we do that in a way that wasn't a perf regression but still gave us data we need to track crashes?
,
May 4 2016
Comment 15 shows that CHECK still does a lot more even if there aren't any stream parameters. I don't think Android really needs this special case any more. We added microdumps to Chrome to handle the user feedback case that motivated https://codereview.chromium.org/336413005 - hopefully this is enough. We should probably just treat that completely separately to the blink perf issue here: check what the binary size cost of keeping the strings is on Android, and then just decide whether we think that cost is worth keeping these strings for in-the-field debugging of official builds. It does appear that this change isn't actually enough to resolve the blink perf issues, though: it won't fix any non-android regressions, and it also doesn't make regular non-official release builds faster...
,
May 4 2016
#15 was about the android (aka non-official-build) version of CHECK.
,
May 4 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b9d5931295b7eab7bd2f029b8d896082b911d776 commit b9d5931295b7eab7bd2f029b8d896082b911d776 Author: danakj <danakj@chromium.org> Date: Wed May 04 20:06:31 2016 Stop compiling and printing logging strings for CHECK() on Android. This is a performance regression in general, and causes visible regression when Blink used CHECK instead of their own RELEASE_ASSERT. This behaviour was introduced in https://codereview.chromium.org/336413005 If Android continues to need something special here (radio silence from previously-interested parties for the last few weeks), we should print __FILE__ and __LINE__ or something, and not support the full string logging that this patch reverts. R=haraken, thakis@chromium.org BUG= 599867 , 596760 , 378974 Review-Url: https://codereview.chromium.org/1937613002 Cr-Commit-Position: refs/heads/master@{#391613} [modify] https://crrev.com/b9d5931295b7eab7bd2f029b8d896082b911d776/base/logging.h
,
May 5 2016
Yes, I know #15 is about the non-official version - that was my point, the version that includes the filename and line number is still more expensive than RELEASE_ASSERT, even when you don't include any stream parameters, so just including the filename and line number is still going to be a perf regression.
,
May 5 2016
On a local official build this change saves 252KiB of binary size for arm32, a 0.6% reduction. It presumably also has some performance benefit, but I haven't tried to measure that. I think that might be a saving worth having given that we now have microdumps in the logs to help us diagnose problems in cases where we don't get a minidump uploaded. The situation where we neither have a minidump, nor an intact microdump, but do have enough of the CHECK/LOG output to figure it out anyway, is probably pretty rare?
,
May 5 2016
Ah I see! Thanks for the followup on the savings. I'm happy with leaving things where they have settled now.
,
May 17 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/481a8ec8b24df24795c63fd4ec26f3670d516db8 commit 481a8ec8b24df24795c63fd4ec26f3670d516db8 Author: thakis <thakis@chromium.org> Date: Tue May 17 03:09:30 2016 In official builds, let CHECK(false) crash instead of calling BreakDebugger. This should save some binary size and make things a bit faster, without ill effects. See bug comment 15, and brettw's and my comments on "[blink-dev] Update of wtf/Assertions.h, and ASSERT macros deprecation" BUG= 599867 Review-Url: https://codereview.chromium.org/1982123002 Cr-Commit-Position: refs/heads/master@{#394035} [modify] https://crrev.com/481a8ec8b24df24795c63fd4ec26f3670d516db8/base/logging.h
,
May 24 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4d0d91ff393599cb9e605b10b09eb59983145cd4 commit 4d0d91ff393599cb9e605b10b09eb59983145cd4 Author: kotenkov <kotenkov@yandex-team.ru> Date: Tue May 24 17:24:49 2016 Replace all occurrences of RELEASE_ASSERT in wtf with CHECK. This is a reland of https://codereview.chromium.org/1840163002. Performance regressions were addressed in https://codereview.chromium.org/1937613002 and https://codereview.chromium.org/1840163002. BUG= 596760 , 599867 Review-Url: https://codereview.chromium.org/1992873004 Cr-Commit-Position: refs/heads/master@{#395624} [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/Deque.h [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/Functional.h [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/HashTable.h [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/TerminatedArrayBuilder.h [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/Vector.h [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/WTF.cpp [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/allocator/PageAllocator.cpp [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/allocator/PartitionAlloc.cpp [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/allocator/PartitionAlloc.h [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/allocator/PartitionAllocator.h [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/allocator/Partitions.cpp [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/dtoa/utils.h [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/text/AtomicString.cpp [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/text/CString.cpp [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/text/StringConcatenate.cpp [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/text/StringConcatenate.h [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/text/StringImpl.cpp [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/text/StringImpl.h [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/text/TextCodecUTF16.cpp [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/text/WTFString.cpp [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/typed_arrays/ArrayBuffer.h [modify] https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4/third_party/WebKit/Source/wtf/typed_arrays/TypedArrayBase.h
,
May 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/90ab609c3dcf497cb08af396f552ca1c8c56e10a commit 90ab609c3dcf497cb08af396f552ca1c8c56e10a Author: kotenkov <kotenkov@yandex-team.ru> Date: Fri May 27 11:46:46 2016 Revert of Replace all occurrences of RELEASE_ASSERT in wtf with CHECK. (patchset #3 id:40001 of https://codereview.chromium.org/1992873004/ ) Reason for revert: 12.5%-56.9% regression in blink_perf.layout at 395611:395672 Original issue's description: > Replace all occurrences of RELEASE_ASSERT in wtf with CHECK. > > This is a reland of https://codereview.chromium.org/1840163002. > > Performance regressions were addressed in > https://codereview.chromium.org/1937613002 > and https://codereview.chromium.org/1840163002. > > BUG= 596760 , 599867 > > Committed: https://crrev.com/4d0d91ff393599cb9e605b10b09eb59983145cd4 > Cr-Commit-Position: refs/heads/master@{#395624} TBR=haraken@chromium.org,yutak@chromium.org,tkent@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 596760 , 599867 , 614852 Review-Url: https://codereview.chromium.org/2016223002 Cr-Commit-Position: refs/heads/master@{#396441} [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/Deque.h [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/Functional.h [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/HashTable.h [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/TerminatedArrayBuilder.h [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/Vector.h [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/WTF.cpp [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/allocator/PageAllocator.cpp [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/allocator/PartitionAlloc.cpp [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/allocator/PartitionAlloc.h [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/allocator/PartitionAllocator.h [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/allocator/Partitions.cpp [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/dtoa/utils.h [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/text/AtomicString.cpp [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/text/CString.cpp [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/text/StringConcatenate.cpp [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/text/StringConcatenate.h [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/text/StringImpl.cpp [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/text/StringImpl.h [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/text/TextCodecUTF16.cpp [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/text/WTFString.cpp [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/typed_arrays/ArrayBuffer.h [modify] https://crrev.com/90ab609c3dcf497cb08af396f552ca1c8c56e10a/third_party/WebKit/Source/wtf/typed_arrays/TypedArrayBase.h
,
Jun 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6ff194f572f62ff4989aaadc9af6616eb78b954a commit 6ff194f572f62ff4989aaadc9af6616eb78b954a Author: primiano <primiano@chromium.org> Date: Mon Jun 06 19:13:45 2016 Revert of In official builds, let CHECK(false) crash instead of calling BreakDebugger. (patchset #2 id:20001 of https://codereview.chromium.org/1982123002/ ) Reason for revert: Unfortunately crrev.com/1982123002 causes loss of crash reports on Android arm64 (and supposedly also CrOS). This is because __builtin_trap() raises a SIGILL on x86 and arm but SIGTRAP on arm64. Breakpad does not handle SIGTRAP (yet). Temporarily reverting this CL until SIGTRAP support for breakpad lands. BUG= 599867 ,614865 Original issue's description: > In official builds, let CHECK(false) crash instead of calling BreakDebugger. > > This should save some binary size and make things a bit faster, without ill > effects. > > See bug comment 15, and brettw's and my comments on > "[blink-dev] Update of wtf/Assertions.h, and ASSERT macros deprecation" > > BUG= 599867 > > Committed: https://crrev.com/481a8ec8b24df24795c63fd4ec26f3670d516db8 > Cr-Commit-Position: refs/heads/master@{#394035} TBR=danakj@chromium.org,thakis@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 599867 Review-Url: https://codereview.chromium.org/2046593002 Cr-Commit-Position: refs/heads/master@{#398084} [modify] https://crrev.com/6ff194f572f62ff4989aaadc9af6616eb78b954a/base/logging.h
,
Jun 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0a508dd39b250ccac9c73b39d04e7cbbe5dfc74d commit 0a508dd39b250ccac9c73b39d04e7cbbe5dfc74d Author: Primiano Tucci <primiano@chromium.org> Date: Mon Jun 06 19:37:43 2016 Revert of In official builds, let CHECK(false) crash instead of calling BreakDebugger. (patchset #2 id:20001 of https://codereview.chromium.org/1982123002/ ) Reason for revert: Unfortunately crrev.com/1982123002 causes loss of crash reports on Android arm64 (and supposedly also CrOS). This is because __builtin_trap() raises a SIGILL on x86 and arm but SIGTRAP on arm64. Breakpad does not handle SIGTRAP (yet). Temporarily reverting this CL until SIGTRAP support for breakpad lands. BUG= 599867 ,614865 Original issue's description: > In official builds, let CHECK(false) crash instead of calling BreakDebugger. > > This should save some binary size and make things a bit faster, without ill > effects. > > See bug comment 15, and brettw's and my comments on > "[blink-dev] Update of wtf/Assertions.h, and ASSERT macros deprecation" > > BUG= 599867 > > Committed: https://crrev.com/481a8ec8b24df24795c63fd4ec26f3670d516db8 > Cr-Commit-Position: refs/heads/master@{#394035} TBR=danakj@chromium.org,thakis@chromium.org BUG= 599867 Review-Url: https://codereview.chromium.org/2046593002 Cr-Commit-Position: refs/heads/master@{#398084} (cherry picked from commit 6ff194f572f62ff4989aaadc9af6616eb78b954a) Review URL: https://codereview.chromium.org/2042003002 . Cr-Commit-Position: refs/branch-heads/2743@{#245} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/0a508dd39b250ccac9c73b39d04e7cbbe5dfc74d/base/logging.h
,
Jul 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ba910a65a5a63594dc8558b337c3cb12028bc4fd commit ba910a65a5a63594dc8558b337c3cb12028bc4fd Author: primiano <primiano@chromium.org> Date: Thu Jul 07 22:14:48 2016 Reland of In official builds, let CHECK(false) crash instead of calling BreakDebugger. (https://codereview.chromium.org/2046593002/ ) Reason for relanding: The SIGTRAP support has landed in breakpad (https://codereview.chromium.org/2068673002) and the crash reports seem to flow through correctly. Should now be safe to rely on __builtin_trap() on arm64. Original issue's description: > Revert of In official builds, let CHECK(false) crash instead of calling BreakDebugger. (patchset #2 id:20001 of https://codereview.chromium.org/1982123002/ ) > > Reason for revert: > Unfortunately crrev.com/1982123002 causes loss of > crash reports on Android arm64 (and supposedly also CrOS). > This is because __builtin_trap() raises a SIGILL on x86 and > arm but SIGTRAP on arm64. Breakpad does not handle SIGTRAP (yet). > Temporarily reverting this CL until SIGTRAP support for breakpad lands. > > BUG= 599867 ,614865 > > Original issue's description: > > In official builds, let CHECK(false) crash instead of calling BreakDebugger. > > > > This should save some binary size and make things a bit faster, without ill > > effects. > > > > See bug comment 15, and brettw's and my comments on > > "[blink-dev] Update of wtf/Assertions.h, and ASSERT macros deprecation" > > > > BUG= 599867 > > > > Committed: https://crrev.com/481a8ec8b24df24795c63fd4ec26f3670d516db8 > > Cr-Commit-Position: refs/heads/master@{#394035} > > TBR=danakj@chromium.org,thakis@chromium.org > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG= 599867 > > Committed: https://crrev.com/6ff194f572f62ff4989aaadc9af6616eb78b954a > Cr-Commit-Position: refs/heads/master@{#398084} BUG= 599867 ,614865 Review-Url: https://codereview.chromium.org/2125923002 Cr-Commit-Position: refs/heads/master@{#404249} [modify] https://crrev.com/ba910a65a5a63594dc8558b337c3cb12028bc4fd/base/logging.h
,
Feb 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8c972d0e190168b4b5621e81563f319563fd0af8 commit 8c972d0e190168b4b5621e81563f319563fd0af8 Author: primiano <primiano@chromium.org> Date: Fri Feb 17 21:08:50 2017 base: make CHECK macros trap at distinct addresses in official builds Abstract -------- CHECK() is the macro used all over the places (~4K occurrences without counting for dupes due to inlining) for release-time assertions. It is enabled in a minimal form (crash without a message) in official builds. It needs to be fast as it is used in lot of fastpath. It needs to not emit too much code, as it is used in lot of places. It needs to guarantee that crash reports can pinpoint to the right location when hitting a CHECK. - Back in the days CHECK was fat, slow, but crash-friendly. - After crrev.com/1982123002 it became tiny, fast but not crash friendly. - This CL is making it a bit less tiny (+28/+128K), fast and crash friendly. The problem this CL deals with is the case of multiple CHECK()s within the same function. Also, adds a test that covers Mac, Linux and Android. A bit of history: ----------------- Before crrev.com/1982123002 (reverted and later re-landed in crrev.com/2125923002) CHECK() in official builds was essentially: if (!condition) BreakDebugger() It was later found that this approach was not efficient, both in terms of binary size and performance. More importantly it was a regression w.r.t. blink's assert that were later switched to CHECK(). "[blink-dev] Update of wtf/Assertions.h, and ASSERT macros deprecation" The major reason for this is DebuggerBreak being quite complex and not treated as noreturn by the compiler. It seems (see crbug.com/664209 for more) that the most efficient way to handle these checks is ending up with: Source code: CHECK(cond1); CHECK(cond2); ... Ideal assembly: (ideal for perf and binary, but not for crash reports, more below) compare_opcode cond1; jump_if_zero prologue; compare_opcode cond2; jump_if_zero prologue; ... prologue: trap_instruction(s) Rather than something like: compare_opcode cond1; jump_if_NOT_zero next1; trap_instruction(s) next1: compare_opcode cond2; jump_if_NOT_zero next2; trap_instruction(s) next2: ... Where essentially the trap instructions are interleaved within the main execution flow. This is even worse if the trap instruction is actually a call to a function, with annex frame initialization, as in the case of BreakDebugger(). That bloats the binary and reduces i-cache hotness for the main flow. crrev.com/1982123002 recently fixed the situation making the assembly look like the ideal case above. Unfortunately this caused another problem due the extreme optimization: once the program crashes in "trap_instruction(s)", there is no easy way to tell which condition caused the trap. In practice this translates into the inability of tell which CHECK failed in a function that has more than one check. This CL: -------- Re-addresses crrev.com/2125923002, adding an extra instruction after the trap which creates an opcode with a unique counter. This prevents the compiler from folding the trap instructions, still applying no-return optimizations. Also by doing this the various prologue get properly attributed to the CHECK line in debugging symbols. Binary size inflation on official builds: ----------------------------------------- Android arm: 48684276 -> 48712948 = 28 K Android arm64: 85611800 -> 85665048 = 53 K Android x86_64: 91904944 -> 91933616 = 28 K Linux x86_64: 124219488 -> 124346464 = 124 K (Android build with -Os, hence why the difference between the two) BUG= 664209 , 599867 Review-Url: https://codereview.chromium.org/2502953003 Cr-Commit-Position: refs/heads/master@{#451381} [modify] https://crrev.com/8c972d0e190168b4b5621e81563f319563fd0af8/base/logging.h [modify] https://crrev.com/8c972d0e190168b4b5621e81563f319563fd0af8/base/logging_unittest.cc
,
Feb 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4c98a8c2c794b10f70fc22e02128139252a0b0c1 commit 4c98a8c2c794b10f70fc22e02128139252a0b0c1 Author: alph <alph@chromium.org> Date: Fri Feb 17 21:29:15 2017 Revert of base: make CHECK macros trap at distinct addresses in official builds (patchset #10 id:180001 of https://codereview.chromium.org/2502953003/ ) Reason for revert: Broke compile https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20ChromeOS/builds/29509 Original issue's description: > base: make CHECK macros trap at distinct addresses in official builds > > Abstract > -------- > CHECK() is the macro used all over the places (~4K occurrences without > counting for dupes due to inlining) for release-time assertions. > It is enabled in a minimal form (crash without a message) in official builds. > It needs to be fast as it is used in lot of fastpath. > It needs to not emit too much code, as it is used in lot of places. > It needs to guarantee that crash reports can pinpoint to the right > location when hitting a CHECK. > > - Back in the days CHECK was fat, slow, but crash-friendly. > - After crrev.com/1982123002 it became tiny, fast but not crash friendly. > - This CL is making it a bit less tiny (+28/+128K), fast and crash friendly. > > The problem this CL deals with is the case of multiple CHECK()s within > the same function. Also, adds a test that covers Mac, Linux and Android. > > A bit of history: > ----------------- > Before crrev.com/1982123002 (reverted and later re-landed in > crrev.com/2125923002) CHECK() in official builds was essentially: > if (!condition) BreakDebugger() > It was later found that this approach was not efficient, both in terms > of binary size and performance. More importantly it was a regression > w.r.t. blink's assert that were later switched to CHECK(). > "[blink-dev] Update of wtf/Assertions.h, and ASSERT macros deprecation" > The major reason for this is DebuggerBreak being quite complex and > not treated as noreturn by the compiler. > It seems (see crbug.com/664209 for more) that the most efficient way to > handle these checks is ending up with: > > Source code: > CHECK(cond1); > CHECK(cond2); > ... > > Ideal assembly: > (ideal for perf and binary, but not for crash reports, more below) > compare_opcode cond1; > jump_if_zero prologue; > compare_opcode cond2; > jump_if_zero prologue; > ... > prologue: > trap_instruction(s) > > Rather than something like: > compare_opcode cond1; > jump_if_NOT_zero next1; > trap_instruction(s) > next1: > compare_opcode cond2; > jump_if_NOT_zero next2; > trap_instruction(s) > next2: > ... > Where essentially the trap instructions are interleaved within the > main execution flow. This is even worse if the trap instruction is > actually a call to a function, with annex frame initialization, > as in the case of BreakDebugger(). That bloats the binary and > reduces i-cache hotness for the main flow. > > crrev.com/1982123002 recently fixed the situation making the > assembly look like the ideal case above. Unfortunately this caused > another problem due the extreme optimization: once the program > crashes in "trap_instruction(s)", there is no easy way to tell which condition > caused the trap. In practice this translates into the inability of > tell which CHECK failed in a function that has more than one check. > > This CL: > -------- > Re-addresses crrev.com/2125923002, adding an extra instruction after > the trap which creates an opcode with a unique counter. This prevents > the compiler from folding the trap instructions, still applying no-return > optimizations. > Also by doing this the various prologue get properly attributed to the > CHECK line in debugging symbols. > > Binary size inflation on official builds: > ----------------------------------------- > Android arm: 48684276 -> 48712948 = 28 K > Android arm64: 85611800 -> 85665048 = 53 K > Android x86_64: 91904944 -> 91933616 = 28 K > Linux x86_64: 124219488 -> 124346464 = 124 K > (Android build with -Os, hence why the difference between the two) > > BUG= 664209 , 599867 > > Review-Url: https://codereview.chromium.org/2502953003 > Cr-Commit-Position: refs/heads/master@{#451381} > Committed: https://chromium.googlesource.com/chromium/src/+/8c972d0e190168b4b5621e81563f319563fd0af8 TBR=thakis@chromium.org,torne@chromium.org,brettw@chromium.org,danakj@chromium.org,mark@chromium.org,primiano@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 664209 , 599867 Review-Url: https://codereview.chromium.org/2706453004 Cr-Commit-Position: refs/heads/master@{#451384} [modify] https://crrev.com/4c98a8c2c794b10f70fc22e02128139252a0b0c1/base/logging.h [modify] https://crrev.com/4c98a8c2c794b10f70fc22e02128139252a0b0c1/base/logging_unittest.cc
,
Feb 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f53167270c44da971a2f085299404319e42756f4 commit f53167270c44da971a2f085299404319e42756f4 Author: primiano <primiano@chromium.org> Date: Tue Feb 21 13:09:26 2017 [Reland] base: make CHECK macros trap at distinct addresses in official builds Original CL: https://crrev.com/2502953003 Revert: https://crrev.com/2706453004 Reason for relanding: masked out OS_NACL which was failing to build because of this error: "GNU-style inline assembly is disabled" Original CL Description: > Abstract > -------- > CHECK() is the macro used all over the places (~4K occurrences without > counting for dupes due to inlining) for release-time assertions. > It is enabled in a minimal form (crash without a message) in official builds. > It needs to be fast as it is used in lot of fastpath. > It needs to not emit too much code, as it is used in lot of places. > It needs to guarantee that crash reports can pinpoint to the right > location when hitting a CHECK. > > - Back in the days CHECK was fat, slow, but crash-friendly. > - After crrev.com/1982123002 it became tiny, fast but not crash friendly. > - This CL is making it a bit less tiny (+28/+128K), fast and crash friendly. > > The problem this CL deals with is the case of multiple CHECK()s within > the same function. Also, adds a test that covers Mac, Linux and Android. > > A bit of history: > ----------------- > Before crrev.com/1982123002 (reverted and later re-landed in > crrev.com/2125923002) CHECK() in official builds was essentially: > if (!condition) BreakDebugger() > It was later found that this approach was not efficient, both in terms > of binary size and performance. More importantly it was a regression > w.r.t. blink's assert that were later switched to CHECK(). > "[blink-dev] Update of wtf/Assertions.h, and ASSERT macros deprecation" > The major reason for this is DebuggerBreak being quite complex and > not treated as noreturn by the compiler. > It seems (see crbug.com/664209 for more) that the most efficient way to > handle these checks is ending up with: > > Source code: > CHECK(cond1); > CHECK(cond2); > ... > > Ideal assembly: > (ideal for perf and binary, but not for crash reports, more below) > compare_opcode cond1; > jump_if_zero prologue; > compare_opcode cond2; > jump_if_zero prologue; > ... > prologue: > trap_instruction(s) > > Rather than something like: > compare_opcode cond1; > jump_if_NOT_zero next1; > trap_instruction(s) > next1: > compare_opcode cond2; > jump_if_NOT_zero next2; > trap_instruction(s) > next2: > ... > Where essentially the trap instructions are interleaved within the > main execution flow. This is even worse if the trap instruction is > actually a call to a function, with annex frame initialization, > as in the case of BreakDebugger(). That bloats the binary and > reduces i-cache hotness for the main flow. > > crrev.com/1982123002 recently fixed the situation making the > assembly look like the ideal case above. Unfortunately this caused > another problem due the extreme optimization: once the program > crashes in "trap_instruction(s)", there is no easy way to tell which condition > caused the trap. In practice this translates into the inability of > tell which CHECK failed in a function that has more than one check. > > This CL: > -------- > Re-addresses crrev.com/2125923002, adding an extra instruction after > the trap which creates an opcode with a unique counter. This prevents > the compiler from folding the trap instructions, still applying no-return > optimizations. > Also by doing this the various prologue get properly attributed to the > CHECK line in debugging symbols. > > Binary size inflation on official builds: > ----------------------------------------- > Android arm: 48684276 -> 48712948 = 28 K > Android arm64: 85611800 -> 85665048 = 53 K > Android x86_64: 91904944 -> 91933616 = 28 K > Linux x86_64: 124219488 -> 124346464 = 124 K > (Android build with -Os, hence why the difference between the two) > BUG= 664209 , 599867 TBR=mark@chromium.org Review-Url: https://codereview.chromium.org/2705053002 Cr-Commit-Position: refs/heads/master@{#451749} [modify] https://crrev.com/f53167270c44da971a2f085299404319e42756f4/base/logging.h [modify] https://crrev.com/f53167270c44da971a2f085299404319e42756f4/base/logging_unittest.cc |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by majidvp@chromium.org
, Apr 1 2016