bench: Move generated data to a dedicated translation unit #16299

promag commented at 2:16 PM on June 27, 2019: member

With this change multiple benchmarks can use the same data without incurring in a bigger binary.

fanquake added the label Tests on Jun 27, 2019

promag force-pushed on Jun 27, 2019

MarcoFalke commented at 2:56 PM on June 27, 2019: member

Does not compile

promag commented at 3:06 PM on June 27, 2019: member

You must make clean or rm src/bench/data/block413567.raw.h.

MarcoFalke commented at 3:10 PM on June 27, 2019: member

Please see the travis output, there are missing files.

promag force-pushed on Jun 27, 2019

promag commented at 3:14 PM on June 27, 2019: member

🤦‍♂ ... fixed.

promag force-pushed on Jun 27, 2019

MarcoFalke added the label Refactoring on Jun 27, 2019

DrahtBot commented at 5:09 PM on June 27, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16267 (bench: Benchmark blockToJSON by fanatid)
#16116 (Avoid unnecessary signing provider copies on descriptor expansion by Empact)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

promag force-pushed on Jun 27, 2019

promag commented at 12:06 AM on June 28, 2019: member

Not sure how to fix msvc build:

c:\projects\bitcoin\src\bench\data\block413567.raw.h(1): error C2373: 'benchmark::data::block413567': redefinition; different type modifiers [C:\projects\bitcoin\build_msvc\bench_bitcoin\bench_bitcoin.vcxproj]

in build_msvc/bench_bitcoin/bench_bitcoin.vcxproj:26 in 9a69bf9ac2 outdated

  19 | @@ -20,6 +20,7 @@
  20 |      <ClCompile Include="..\..\src\bench\checkqueue.cpp" />
  21 |      <ClCompile Include="..\..\src\bench\coin_selection.cpp" />
  22 |      <ClCompile Include="..\..\src\bench\crypto_hash.cpp" />
  23 | +    <ClCompile Include="..\..\src\bench\data.cpp" />
  24 |      <ClCompile Include="..\..\src\bench\examples.cpp" />
  25 |      <ClCompile Include="..\..\src\bench\lockedpool.cpp" />
  26 |      <ClCompile Include="..\..\src\bench\mempool_eviction.cpp" />

MarcoFalke commented at 3:09 AM on June 28, 2019:

diff --git a/build_msvc/bench_bitcoin/bench_bitcoin.vcxproj b/build_msvc/bench_bitcoin/bench_bitcoin.vcxproj
index 6ded6895cd..4831c4d514 100644
--- a/build_msvc/bench_bitcoin/bench_bitcoin.vcxproj
+++ b/build_msvc/bench_bitcoin/bench_bitcoin.vcxproj
@@ -68,9 +68,9 @@
     <ItemGroup>
       <RawBenchFile Include="..\..\src\bench\data\*.raw" />
     </ItemGroup>
-    <HeaderFromHexdump RawFilePath="%(RawBenchFile.FullPath)" HeaderFilePath="%(RawBenchFile.FullPath).h" SourceHeader="static unsigned const char %(RawBenchFile.Filename)[] = {" SourceFooter="};" />
+    <HeaderFromHexdump RawFilePath="%(RawBenchFile.FullPath)" HeaderFilePath="%(RawBenchFile.FullPath).h" SourceHeader="const std::vector<uint8_t> %(RawBenchFile.Filename)[] = {" SourceFooter="};" />
   </Target>
   <Import Label="hexdumpTarget" Project="..\msbuild\tasks\hexdump.targets" />
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.props" />
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
-</Project>
\ No newline at end of file
+</Project>

promag commented at 8:15 PM on June 30, 2019:

I still can't find a way to fix it 😕

promag force-pushed on Jun 28, 2019

promag force-pushed on Jun 29, 2019

promag force-pushed on Jun 30, 2019

MarcoFalke commented at 12:12 PM on July 1, 2019: member

The windows build just fails without any error message:

C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\Common7\IDE\VC\VCTargets\Microsoft.CppCommon.targets(423,5): error MSB6006: "clcache.exe" exited with code 1. [C:\projects\bitcoin\build_msvc\bench_bitcoin\bench_bitcoin.vcxproj]

@sipsorcery Any ideas?

sipsorcery commented at 7:10 PM on July 1, 2019: member

@MarcoFalke the almost 1 million byte vector initialiser list kills the Microsoft C++ compiler. The Visual Studio 2017 version crashes when compiling data.cpp nce the initialiser list gets close to 80k items. The Visual Studio 2019 version didn't crash for with the full 1 million items within approx 5 minutes at which point I killed it.

There is no useful error message or documentation I could find as to why the compiler doesn't like initialiser lists.

What I can verify is that what causes the problem is switching from: static unsigned const char block413567[] to extern const std::vector<uint8_t> block413567;

Compiling with a 1 million element array works well and is quick. With the vector cl.exe either cannot cope or takes an inordinate amount of time. I'd recommend sticking with the array for such a large amount of data.

in src/bench/data.h:8 in e0bf71d232 outdated

   0 | @@ -0,0 +1,21 @@
   1 | +// Copyright (c) 2019 The Bitcoin Core developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#ifndef BITCOIN_BENCH_DATA_H
   6 | +#define BITCOIN_BENCH_DATA_H
   7 | +
   8 | +#include <bench/data.h>

sipsorcery commented at 7:11 PM on July 1, 2019:

This looks like a recursive include?

promag commented at 7:32 PM on July 1, 2019: member

@sipsorcery thanks! I'll work around it.

promag force-pushed on Jul 1, 2019

promag commented at 9:36 PM on July 1, 2019: member

@MarcoFalke updated, msvc is happy now, @sipsorcery thanks a lot!

in src/bench/data.h:14 in d77899f266 outdated

   9 | +#include <vector>
  10 | +
  11 | +namespace benchmark {
  12 | +namespace data {
  13 | +
  14 | +extern const std::vector<uint8_t>& block413567;

MarcoFalke commented at 3:30 PM on July 2, 2019:

extern const std::vector<uint8_t> block413567;

We generally do it without the &, no?

MarcoFalke commented at 3:31 PM on July 2, 2019: member

Fine with me. While we don't ship the bench binary and have no space concerns, this shouldn't hurt either.

promag force-pushed on Jul 2, 2019

bench: Move generated data to a dedicated translation unit 3d60a03a7c

promag force-pushed on Jul 2, 2019

laanwj commented at 1:39 PM on July 3, 2019: member

Fine with me. While we don't ship the bench binary and have no space concerns, this shouldn't hurt either.

Yes, I think this is good habit. And might also reduce total parsing/compile time.

code review ACK 3d60a03a7cfb2d46b5f10633e9f6a9a36b8cb76f

MarcoFalke referenced this in commit 91c345eb92 on Jul 3, 2019

MarcoFalke merged this on Jul 3, 2019

MarcoFalke closed this on Jul 3, 2019

promag deleted the branch on Jul 3, 2019

deadalnix referenced this in commit 33b30bc21b on May 6, 2020

random-zebra referenced this in commit 26e90b014e on Jul 21, 2021

random-zebra referenced this in commit b4751e10ce on Aug 11, 2021

kittywhiskers referenced this in commit acf58ff0b0 on Nov 6, 2021

kittywhiskers referenced this in commit d920018f06 on Nov 30, 2021

kittywhiskers referenced this in commit 770b697da4 on Dec 3, 2021

kittywhiskers referenced this in commit 7ab0e85ee3 on Dec 4, 2021

kittywhiskers referenced this in commit a273bec000 on Dec 6, 2021

kittywhiskers referenced this in commit 55e0d56592 on Dec 8, 2021

kittywhiskers referenced this in commit f2ff3e442f on Dec 8, 2021

kittywhiskers referenced this in commit 6ed835cbda on Dec 8, 2021

kittywhiskers referenced this in commit ac996a545d on Dec 11, 2021

kittywhiskers referenced this in commit fbbe59068c on Dec 12, 2021

kittywhiskers referenced this in commit a1c6460c97 on Dec 12, 2021

kittywhiskers referenced this in commit 6a6a262ba7 on Dec 12, 2021

kittywhiskers referenced this in commit b0cc1e2772 on Dec 12, 2021

kittywhiskers referenced this in commit 195772068e on Dec 12, 2021

kittywhiskers referenced this in commit cd23b0f8e8 on Dec 12, 2021

kittywhiskers referenced this in commit e51c4c41c0 on Dec 12, 2021

DrahtBot locked this on Dec 16, 2021