Issue metadata
Sign in to add a comment
|
6.2%-6.4% regression in rasterize_and_record_micro.top_25_smooth at 380376:380384 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Mar 10 2016
=== Auto-CCing suspected CL author primiano@chromium.org === Hi primiano@chromium.org, 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 : Enable the allocator shim on Linux Desktop / CrOS Author : primiano Commit description: This CL enables the shim introduced in crrev.com/1675143004 by default, only Linux desktop and CrOS. This CL is functionally a no-op, in the sense that does not change the default allocator (which remains tcmalloc on Linux/CrOS) and does not introduce any new feature (other than the possibility, for future CLs to install hooks in the allocator path). See crrev.com/1675143004 for a longer description about the allocator shim and the design doc bit.ly/allocator-shim. Note for stability sheriffs --------------------------- If you see suspicious crashes in tcmalloc (especially in free/tc_free) there are pretty good chances that something unexpectedly went wrong here and this CL is the culprit. Note for perf sheriffs ---------------------- There is a possibility that this CL might cause a regression on the perf waterfalls (only on Linux/CrOS bots) due to crbug.com/593344 . The telemetry tests I tried to run locally were all inconclusive. Should a regression happen, a temporary workaround is possible as discussed in https://codereview.chromium.org/1675143004/#msg38. BUG=550886 TEST=base_unittests (AllocatorShimTest.*,OutOfMemoryDeathTest.*,TCMalloc*) Review URL: https://codereview.chromium.org/1781573002 Cr-Commit-Position: refs/heads/master@{#380377} Commit : 8712d70e87c419af545e2bad088126d0e19eb384 Date : Thu Mar 10 09:19:17 2016 ===== TESTED REVISIONS ===== Revision Mean Value Std. Dev. Num Values Good? chromium@380375 0.237594 0.000376 8 good chromium@380376 0.237318 0.000645 8 good chromium@380377 0.2539 0.000201 5 bad chromium@380378 0.254167 0.000861 5 bad chromium@380380 0.254767 0.002138 5 bad chromium@380384 0.253797 0.00068 8 bad Bisect job ran on: linux_perf_bisect Bug ID: 593872 Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --also-run-disabled-tests rasterize_and_record_micro.top_25_smooth Test Metric: record_time/record_time Relative Change: 6.83% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bisect/builds/6303 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9018546545859357648 | 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 label Cr-Tests-AutoBisect. Thank you!
,
Mar 11 2016
Yup I know. See my recent email to perf-sheriffs@ (https://groups.google.com/a/chromium.org/d/msg/perf-sheriffs/iYdqGExyCXY/FBJ9FZkyCwAJ) ----- TL;DR If you are not perf-sheriffing you can stop reading here. crrev.com/1675143004 (#380196) caused a perf regression on linux-release. crrev.com/1777363002 should fix it As speculated in the commit message, crrev.com/1675143004 ended up causing a visible across-the board perf regression (example) due to crbug.com/593344 . I was hoping that would have not been visible (the local benchmarks I tried before landing were noisy), but that was not the case. Not sure if the GASP alert system realized that already, I think it will do that soon. The regression is linux-only. I have a cure pending review in, that should fix it crrev.com/1777363002. So, if you are sheriffing and see an across-the-board perf regression on Linux around #380196, (I guess all page cyclers might be affected) that's me and should go away once crrev.com/1777363002 lands. Apology for the inconvenience, I tried to be as much proactive as possible but... perf benchmarks are hard. -----
,
Mar 11 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d76421171daa1327f8e1c10ee710ebf911a90d2f commit d76421171daa1327f8e1c10ee710ebf911a90d2f Author: primiano <primiano@chromium.org> Date: Fri Mar 11 11:21:49 2016 Improve performance of the malloc shim layer This fixes the perf regression introduced by crrev.com/1781573002 As speculated, crbug.com/593344 (NoBarrier_Load being double-fenced) causes a visible perf regression, as it causes the addition of two fences in the malloc fast path. This CL adds a workaraound that falls-back on a raw volatile ptr read on Linux+Clang, relying on the fact that on the architectures we care about a load of an aligned pointer is intrinsically atomic [1,2]. Perf regression: https://chromeperf.appspot.com/report?sid=1237900c90f9c5e5320a87af8d9e8828fbd1794af07f0a4a335f1fc2a45f120a&start_rev=379480&end_rev=380417 I verified manually that the regression goes away on cc_perftests with this patch. [1] Chapter 7 of Part 3A - System Programming Guide http://download.intel.com/design/processor/manuals/253668.pdf [2] A3.5.3 Atomicity in the ARM architecture, ARM Architecture Reference Manual, ARMv7-A and ARMv7-R edition http://liris.cnrs.fr/~mmrissa/lib/exe/fetch.php?media=armv7-a-r-manual.pdf BUG=550886, 593872 TEST=cc_perftestsfast --gtest_filter=*PrepareTiles* Review URL: https://codereview.chromium.org/1777363002 Cr-Commit-Position: refs/heads/master@{#380600} [modify] https://crrev.com/d76421171daa1327f8e1c10ee710ebf911a90d2f/base/allocator/allocator_shim.cc
,
Mar 14 2016
Looks like all alerts are recovered. chrisphan@ I'll mark this as fixed. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by chrisphan@chromium.org
, Mar 10 2016