Page MenuHomePhabricator

Split benchmarker into different functions
AbandonedPublic

Authored by wdede on Oct 15 2020, 5:06 PM.

Details

Summary

benchmarker refactor

Test Plan

TBD

Revert Plan

N/A

Diff Detail

Repository
rG Goldilocks
Lint
Lint Errors

Event Timeline

wdede requested review of this revision.Oct 15 2020, 5:06 PM
Restricted Application completed remote builds in Restricted Buildable.Oct 15 2020, 5:06 PM
ardunster added inline comments.
goldilocks-source/include/goldilocks/goldilocks.hpp
125

If you're running this three times, shouldn't this be 3?

  • Created goldilocks.cpp and added clock function.
Restricted Application completed remote builds in Restricted Buildable.Oct 21 2020, 4:59 PM
  • Moved more code from the old codebase to the new code base.
Restricted Application completed remote builds in Restricted Buildable.Oct 27 2020, 8:20 PM
  • Made Benchmarker class final. (Trying to keep non-leaf classes abstract)
  • Moved classes and classes' functions to respective files
Restricted Application completed remote builds in Restricted Buildable.Oct 28 2020, 6:07 PM
  • Set up the skeleton for the 3 types tests (baby, mama and papa bear). Also created BenchmarkerResult class
Restricted Application completed remote builds in Restricted Buildable.Oct 29 2020, 4:28 PM
  • Fixed an issue where std_dev_adj was inaccessible from the BenchmarkerResult class.
  • TODO: BenchmarkResult object to store data from the clock()
Restricted Application completed remote builds in Restricted Buildable.Oct 30 2020, 7:34 PM
  • Fixed an issue where std_dev_adj was inaccessible from the BenchmarkerResult class.
  • BenchmarkResult class Data memmber access mode
Restricted Application completed remote builds in Restricted Buildable.Oct 30 2020, 7:44 PM
  • Changed the BenchmarkeResult class to a struct and used typedef.(Arguable!)
Restricted Application completed remote builds in Restricted Buildable.Nov 3 2020, 4:46 AM

Please delete the files you copy/pasted over, as they're going to get underfoot for the refactor. I'm not reusing most of the code, as it will be hard to refactor. Delete:

  • goldilocks.hpp
  • testmanager.hpp
  • testsuite.hpp
  • goldilocks.cpp
  • testmanager.cpp
  • testsuite.cpp

Reuse only the specific functions you need.

goldilocks-source/include/goldilocks/BenchmarkerResult.hpp
8–9

No need to typedef this. A typedef is for creating an alias of a typename, and you don't need to do that. BenchmarkResult is fine as the struct name.

118

You can drop the name alias from the end here, as you're not typedef'ing.

goldilocks-source/include/goldilocks/testmanager.hpp
2–8

We're actually dropping this entire file as it is, and replacing it with quite a few other pieces forthcoming. I'd recommend not including testmanager.hpp in the diff at all. Just delete it.

goldilocks-source/include/goldilocks/testsuite.hpp
2–8

This too is going to be completely rewritten, so it should not be included in the new Goldilocks, or in this diff. Delete testsuite.hpp.

goldilocks-tester/CMakeLists.txt
135

This line wouldn't have worked anyway, as it says inlcude_ at the start, rather than include_. We also shouldn't need it.

This revision now requires changes to proceed.Nov 3 2020, 2:58 PM
  • Feeling this but might delete later
Restricted Application completed remote builds in Restricted Buildable.Nov 3 2020, 3:14 PM

So how much is getting rewritten and how much can be cannibalized?

In D352#12157, @wdede wrote:
  • Feeling this but might delete later

I just don't want to commit the old files at all.

So how much is getting rewritten and how much can be cannibalized?

Most of it being rewritten, but individual functions can be cannibalized.

Please delete the files you copy/pasted over, as they're going to get underfoot for the refactor. I'm not reusing most of the code, as it will be hard to refactor. Delete:

  • goldilocks.hpp
  • testmanager.hpp
  • testsuite.hpp
  • goldilocks.cpp
  • testmanager.cpp
  • testsuite.cpp

Reuse only the specific functions you need.

@jcmcdonald we need to talk about your input, just spent one hour on it and I think I need help figuring it out!

Here's the core of where I think you're stuck, @wdede. You're trying to tackle the entire thing as a unit, which is too large to tackle as a whole, and would require you to bring in all these old, outdated pieces.

Think in smaller units. Abandon this diff, and then just build exactly one function: the clock() function (look at the original). That's all. Nothing else. Put it in a Differential. And then we'll go from there.

This comment was removed by wdede.
Restricted Application completed remote builds in Restricted Buildable.Nov 29 2020, 3:23 PM