Use int64_t throughout to facilitate probing larger inputs. by NAThompson · Pull Request #517 · google/benchmark

NAThompson · 2018-01-16T22:13:46Z

No description provided.

googlebot · 2018-01-16T22:13:48Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

NAThompson · 2018-01-16T22:16:10Z

I signed it!

googlebot · 2018-01-16T22:16:11Z

CLAs look good, thanks!

dmah42

There is a cost to forcing int64_t on 32-bit platforms. As such, let's keep this just to where it's necessary. From your issue report that seems to be in defining ranges.

dmah42 · 2018-01-16T23:12:49Z

include/benchmark/benchmark.h

-  State(size_t max_iters, const std::vector<int>& ranges, int thread_i,
-        int n_threads, internal::ThreadTimer* timer,
+  State(size_t max_iters, const std::vector<int64_t>& ranges, int64_t thread_i,
+        int64_t n_threads, internal::ThreadTimer* timer,


i'm not sure we need more than 2^32 threads, do we?

I got a bunch of compiler warnings while only changing the Range arguments. But yes it is ridiculous to have 64-bit threadcounts.

dmah42 · 2018-01-16T23:13:30Z

src/benchmark.cc

  std::unique_ptr<internal::ThreadManager> manager;
  std::vector<std::thread> pool(b.threads - 1);
-  const int repeats =
+  const int64_t repeats =


repetitions certainly shouldn't head north of 2^32.

dmah42 · 2018-01-16T23:14:07Z

src/benchmark_register.cc

 void Benchmark::SetName(const char* name) { name_ = name; }

-int Benchmark::ArgsCnt() const {
+int64_t Benchmark::ArgsCnt() const {


also here: if someone is passing this many args around we're in trouble.

AppVeyorBot · 2018-01-16T23:20:15Z

✅ Build benchmark 957 completed (commit 69b4d7718d by @NAThompson)

NAThompson · 2018-01-17T17:00:18Z

Looks like the threads are passed to the AddRange function:

benchmark/src/benchmark_register.cc:432:12: error: cannot initialize a parameter of type 'std::vector<int64_t> *'
      (aka 'vector<long long> *') with an rvalue of type 'std::vector<int> *'
  AddRange(&thread_counts_, min_threads, max_threads, 2);
           ^~~~~~~~~~~~~~~
benchmark/src/benchmark_register.cc:247:48: note: passing argument to parameter 'dst' here
void Benchmark::AddRange(std::vector<int64_t>* dst, int64_t lo, int64_t hi, int64_t mult)

So the type of the threads looks like it must be the same as the type as the argument to the AddRange, or the AddRange function must be templated on an integer. Do you have a preference?

(The other alternative-that my mental model of this code is unsophisticated and I've totally missed the point-is of course the most likely possibility.)

coveralls · 2018-01-17T17:43:06Z

Coverage increased (+0.06%) to 86.985% when pulling 8d180bb on NAThompson:master into 9f5694c on google:master.

dmah42 · 2018-01-17T18:53:09Z

Oh right: Enthusiastic reuse of code.

Templating AddRange on integer sgtm, given it's a generic utility function.

…tions to integers.

EricWF · 2018-01-17T20:20:17Z

include/benchmark/benchmark.h

    std::string error_message;

-    int64_t iterations;
+    int iterations;


What's this change?

I figured the iteration count wouldn't go past 4 billion. It reverts the previous change, which is admittedly annoying.

EricWF · 2018-01-17T20:20:59Z

src/mutex.h

 class Barrier {
 public:
-  Barrier(int num_threads) : running_threads_(num_threads) {}
+  Barrier(int64_t num_threads) : running_threads_(num_threads) {}


We're never going to have more than 4 billion threads.

EricWF · 2018-01-17T20:21:14Z

src/mutex.h

  Mutex lock_;
  Condition phase_condition_;
-  int running_threads_;
+  int64_t running_threads_;


All these changes for thread counts seem unneeded.

EricWF · 2018-01-17T20:21:34Z

src/statistics.cc

  // All repetitions should be run with the same number of iterations so we
  // can take this information from the first benchmark.
-  int64_t const run_iterations = reports.front().iterations;
+  int const run_iterations = reports.front().iterations;


Why are you doing the reverse here, and changing an int64_t into an int?

dmah42

I'm good with where this is going. I believe @pleroy has expressed concern about putting 64-bit stuff into 32-bit runs, so i'd like their thoughts. I think in this case it's justified.

dmah42 · 2018-01-17T20:19:40Z

include/benchmark/benchmark.h

    std::string error_message;

-    int64_t iterations;
+    int iterations;


this was int64_t before and probably still should be.

dmah42 · 2018-01-17T20:20:13Z

src/benchmark.cc

  report.report_label = results.report_label_;
  // Report the total iterations across all threads.
-  report.iterations = static_cast<int64_t>(iters) * b.threads;
+  report.iterations = static_cast<int>(iters) * b.threads;


same here.. keep as int64_t

dmah42 · 2018-01-17T20:23:28Z

src/json_reporter.cc

  return ss.str();
 }

+std::string FormatKV(std::string const& key, int value) {


if this is for outputting the args (which i don't think we do other than as part of the name) then it should be int64_t value, right?

This fixed a build error. Now either type will work.

J/K, that error went away once I made your other suggested changes.

dmah42 · 2018-01-17T20:23:37Z

src/statistics.cc

  // All repetitions should be run with the same number of iterations so we
  // can take this information from the first benchmark.
-  int64_t const run_iterations = reports.front().iterations;
+  int const run_iterations = reports.front().iterations;


dmah42 · 2018-01-17T20:24:04Z

test/benchmark_test.cc


 static void BM_CalculatePi(benchmark::State& state) {
-  static const int depth = 1024;
+  static const int64_t depth = 1024;


definitely doesn't need to be updated :)

AppVeyorBot · 2018-01-17T20:32:44Z

❌ Build benchmark 958 failed (commit 2a02f88a1d by @NAThompson)

AppVeyorBot · 2018-01-17T20:53:10Z

❌ Build benchmark 959 failed (commit ab7f5f972b by @NAThompson)

AppVeyorBot · 2018-01-17T21:28:18Z

❌ Build benchmark 960 failed (commit 15ee0134b3 by @NAThompson)

dmah42 · 2018-01-17T21:30:59Z

works for me once tests pass and @pleroy and @EricWF have weighed in.

i think there's a broader approach here to templatize range args but i may try that after this lands to see if it works out.

coveralls · 2018-01-18T14:51:04Z

Coverage remained the same at 86.926% when pulling d5f07bf on NAThompson:master into 9f5694c on google:master.

AppVeyorBot · 2018-01-18T17:21:22Z

❌ Build benchmark 961 failed (commit ebba6ca394 by @NAThompson)

coveralls · 2018-01-20T06:16:08Z

Coverage remained the same at 86.926% when pulling 9dcdd39 on NAThompson:master into 9f5694c on google:master.

dmah42 · 2018-05-29T12:19:44Z

I think this PR has been obsoleted by other changes.

Use int64_t throughout to facilitate probing larger inputs.

8d180bb

googlebot added the cla: no label Jan 16, 2018

googlebot added cla: yes and removed cla: no labels Jan 16, 2018

dmah42 requested changes Jan 16, 2018

View reviewed changes

Template 'AddRange' on an integer type. Revert threadcount and repeti…

2dcc334

…tions to integers.

EricWF reviewed Jan 17, 2018

View reviewed changes

dmah42 reviewed Jan 17, 2018

View reviewed changes

Revert threadcount to int in mutex class.

7c0f0ac

More int/int64_t changes in response to review.

d5f07bf

dmah42 approved these changes Jan 17, 2018

View reviewed changes

Fix 32-bit build by static casting to size_t.

9dcdd39

dmah42 mentioned this pull request Mar 9, 2018

What is the critera/timing for creating a new release? #549

Closed

dmah42 closed this May 29, 2018

Conversation

NAThompson commented Jan 16, 2018

Uh oh!

googlebot commented Jan 16, 2018

Uh oh!

NAThompson commented Jan 16, 2018

Uh oh!

googlebot commented Jan 16, 2018

Uh oh!

dmah42 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Jan 16, 2018

Uh oh!

NAThompson commented Jan 17, 2018

Uh oh!

coveralls commented Jan 17, 2018

Uh oh!

dmah42 commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmah42 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NAThompson Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Jan 17, 2018

Uh oh!

AppVeyorBot commented Jan 17, 2018

Uh oh!

AppVeyorBot commented Jan 17, 2018

Uh oh!

dmah42 commented Jan 17, 2018

Uh oh!

coveralls commented Jan 18, 2018

Uh oh!

AppVeyorBot commented Jan 18, 2018

Uh oh!

coveralls commented Jan 20, 2018

Uh oh!

dmah42 commented May 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

NAThompson Jan 17, 2018 •

edited

Loading