RELEASE_ASSERT()s being changed to CHECK()s caused performance regressions in layout perf tests on Windows 64bit. Between 2%-20% regressions were seen.
The actual asm implementation of RELEASE_ASSERT and CHECK is identical, but the compiler output over all of blink was changed somehow by the presence of the unused stream operator on CHECK.
I believe this hints at RELEASE_ASSERT being present in very hot code, which is likely unnecessary and could be hoisted out while providing the same verification. This should translate to real perf wins since such a minute change (actually 0 asm change) was very visible on the bots.
Here's is a run of the bots, where the patch being tested was with CHECK() replacement of RELEASE_ASSERT(): http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06-01_15-50-44
Here's the CL for that perf test run: https://codereview.chromium.org/1992873004/
Here's an email thread discussing this from the POV of "what's wrong with CHECK?" https://groups.google.com/a/chromium.org/d/topic/blink-dev/TL0NkNXIT1w/discussion
It would probably be really interesting to make RELEASE_ASSERT artificially expensive, run these perf tests in pprof, and see which ones are being hammered the most.
RELEASE_ASSERT()s being changed to CHECK()s caused performance regressions in layout perf tests on Windows 64bit. Between 2%-20% regressions were seen.
The actual asm implementation of RELEASE_ASSERT and CHECK is identical, but the compiler output over all of blink was changed somehow by the presence of the unused stream operator on CHECK.
I believe this hints at RELEASE_ASSERT being present in very hot code, which is likely unnecessary and could be hoisted out while providing the same verification. This should translate to real perf wins since such a minute change (actually 0 asm change) was very visible on the bots.
Here's is a run of the bots, where the patch being tested was with CHECK() replacement of RELEASE_ASSERT(): http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06-01_15-50-44
Here's the CL for that perf test run: https://codereview.chromium.org/1992873004/
Here's an email thread discussing this from the POV of "what's wrong with CHECK?" https://groups.google.com/a/chromium.org/d/topic/blink-dev/TL0NkNXIT1w/discussion
pdr suggested that it would probably be really interesting to make RELEASE_ASSERT artificially expensive, run these perf tests in pprof, and see which ones are being hammered the most.
One example is Vector::at(size_t). This function is very small, and very hot.
Though I'm not sure if we can remove RELEASE_ASSERT in at(), we can avoid to call at() like the following change:
for (size_t i = 0; i < vector.size(); ++i)
foo(vector[i]); // operator[] is an alias of at().
↓
for (const auto& t : vector)
foo(t); //
Note that replacing all of the indexed access with range loops is great, but I don't think that allows us to regress the performance of operator[] with CHECK. We need to make CHECK fast, we can't hack around it inside blink.
Note also that post-fork Apple added these same asserts to the iterators for added security. We might consider doing the same thing, which means switching to range based for loops here is not a fix for CHECK being slow.
Comment 1 Deleted