Closed Bug 1396208 Opened 7 years ago Closed 7 years ago

Black box function for GTest platform micro benchmarks

Categories

(Testing :: General, defect)

Product:
Testing
Component:
General
Version:
Version 3
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: hsivonen, Assigned: hsivonen)

References

Details

Attachments

(1 file)

Bug 1396208 - For benchmarking, add a black box function that is opaque to the optimizer.
59 bytes, text/x-review-board-request
froydnj
: review+
Details 
GTest platform micro benchmarks should have a black box function that the C++ optimizer or LTO machinery cannot see through in order to be able to inhibit code elimination in the benchmark loops. It should work for both breaking analysis for both inputs to the operation being benchmarked and its outputs. A function that takes a void* and passes it to an in-line asm block should do it.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Based on how the benchmarks regress and based on godbolt.org, I'm pretty confident that this patch does what it's supposed to do with GCC.

I'm less confident that it works the way it's supposed to with LLVM older than LLVM 5, but I'm pretty confident, based on godbolt.org, that the patch works the way it should with LLVM 5. I don't know what LLVM the clang that we use for Mac builds is based on.

The MSVC stuff is unverified cargo-cult based on Google's code.
(In reply to Henri Sivonen (:hsivonen) from comment #9)
> The MSVC stuff is unverified cargo-cult based on Google's code.

Windows also shows enough of a benchmark regression to believe that this is actually inhibiting some optimizations.
Attachment #8917017 - Flags: review?(nfroyd)
Comment on attachment 8917017 [details]
Bug 1396208 - For benchmarking, add a black box function that is opaque to the optimizer.

The patch here imports code from https://github.com/google/benchmark and then throws most of it away. Does the patch look OK licensing-wise? I imported the list of copyright holders from upstream as-is, trailing whitespace and all, instead of trying to figure out which copyright holders the retained lines of code would be attributable to.
Attachment #8917017 - Flags: feedback?(gerv)
You should probably import the LICENSE file too. Other than that, looks good.

Gerv
Comment on attachment 8917017 [details]
Bug 1396208 - For benchmarking, add a black box function that is opaque to the optimizer.

https://reviewboard.mozilla.org/r/188044/#review201912

I have complaints, but I can't tell to what extent we're modifying files that are imported (presumably the upstream files don't have references to Mozilla's try server?).  So none of these are marked as blocking landing.

::: testing/gtest/benchmark/BlackBox.h:56
(Diff revision 8)
> +// Force the compiler to flush pending writes to global memory. Acts as an
> +// effective read/write barrier

What does this comment even mean?  "Force the compiler" would be a compile-time thing, but "flush pending writes to global memory" would be a runtime thing.  What is this function trying to accomplish?

::: testing/gtest/benchmark/BlackBox.h:66
(Diff revision 8)
> +
> +#endif // _MSC_VER
> +
> +template<class T>
> +MOZ_ALWAYS_INLINE_EVEN_DEBUG T* BlackBox(T* aPtr) {
> +  return reinterpret_cast<T*>(BlackBoxVoidPtr(aPtr));

This could really be `static_cast`, I believe.  Would look a *little* less dodgy to the casual reader and/or automated analysis tool.

::: testing/gtest/benchmark/BlackBox.cpp:21
(Diff revision 8)
> +
> +#include "gtest/BlackBox.h"
> +
> +namespace mozilla {
> +
> +char volatile* UseCharPointer(char volatile* aPtr) {

Does the liberal use of `volatile` here inhibit MSVC's link-time optimizer as well?
Attachment #8917017 - Flags: review?(nfroyd) → review+
Comment on attachment 8917017 [details]
Bug 1396208 - For benchmarking, add a black box function that is opaque to the optimizer.

https://reviewboard.mozilla.org/r/188044/#review202118

::: testing/gtest/benchmark/BlackBox.h:56
(Diff revision 8)
> +// Force the compiler to flush pending writes to global memory. Acts as an
> +// effective read/write barrier

I don't really know. I kept it from the Google code. Since I don't know when one would use this and how, I guess it's best to remove this function.
> You should probably import the LICENSE file too. Other than that, looks good.

Thanks. Added a copy of LICENSE.
Comment on attachment 8917017 [details]
Bug 1396208 - For benchmarking, add a black box function that is opaque to the optimizer.

https://reviewboard.mozilla.org/r/188044/#review202118

> I don't really know. I kept it from the Google code. Since I don't know when one would use this and how, I guess it's best to remove this function.

Removed.
Comment on attachment 8917017 [details]
Bug 1396208 - For benchmarking, add a black box function that is opaque to the optimizer.

https://reviewboard.mozilla.org/r/188044/#review201912

> This could really be `static_cast`, I believe.  Would look a *little* less dodgy to the casual reader and/or automated analysis tool.

Changed.
Comment on attachment 8917017 [details]
Bug 1396208 - For benchmarking, add a black box function that is opaque to the optimizer.

https://reviewboard.mozilla.org/r/188044/#review201912

> Does the liberal use of `volatile` here inhibit MSVC's link-time optimizer as well?

Unclear, because we don't seem to run gtest on pgo builds, so can't compare perfherder data before and after to see if there's an effect.
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c0a033707d7a
For benchmarking, add a black box function that is opaque to the optimizer. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/c0a033707d7a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(In reply to Henri Sivonen (:hsivonen) from comment #9)
> I'm less confident that it works the way it's supposed to with LLVM older
> than LLVM 5, but I'm pretty confident, based on godbolt.org, that the patch
> works the way it should with LLVM 5. I don't know what LLVM the clang that
> we use for Mac builds is based on.

FTR, we use the same clang for our Mac builds that we use for our Linux clang builds.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #22)
> FTR, we use the same clang for our Mac builds that we use for our Linux
> clang builds.

So clang 3.9?

- -

Note to sheriff: It is expected that the benchmarks modified by this patch regress.
Depends on: 1415514
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: