Add support for timing steps and pipelines by SamCarlberg · Pull Request #613 · WPIRoboticsProjects/GRIP

SamCarlberg · 2016-07-07T15:28:59Z

This lets users know which parts of their pipelines take the most time, and how long the pipeline takes as a whole.

SamCarlberg · 2016-07-07T15:35:01Z

ui/src/main/java/edu/wpi/grip/ui/MainWindowController.java

  private Pane aboutPane;
  @FXML
  private StatusBar statusBar;
+  private Label elapsedTimeLabel; // TODO improve the layout of this label


This label isn't centered vertically in the status bar, and it separates the start/stop button from the "Pipeline STATE" text.

I would move the Pipeline status into its own label and then center both (i.e. do not use statusBar.setText() for the status)

SamCarlberg · 2016-07-07T16:36:16Z

It may also be a good idea to collect the timing data and have a UI for displaying statistics about each operation, such as

Mean runtime
Median runtime
Relative mean runtime (% of overall time the mean runtime takes)
Relative median runtime (% of overall time the median runtime takes)

And statistics about the pipeline

GRIP overhead (% of time spent not running any operation)
Hotspot operations (those that run > 1 standard deviation above mean run time)

So for the pipeline pictured in the top comment,

GRIP overhead is 2.5%
Mean operation runtime is 5.03ms (s = 6.39ms)
Blur is a big hotspot (two standard deviations above mean)

codecov-io · 2016-07-07T17:08:11Z

Current coverage is 54.25% (diff: 51.56%)

Merging #613 into master will decrease coverage by 0.22%

@@             master       #613   diff @@
==========================================
  Files           209        218     +9   
  Lines          6713       7067   +354   
  Methods           0          0          
  Messages          0          0          
  Branches        656        697    +41   
==========================================
+ Hits           3657       3834   +177   
- Misses         2886       3062   +176   
- Partials        170        171     +1

Powered by Codecov. Last update 1d02ed1...3d7481c

JLLeitschuh · 2016-07-07T18:53:14Z

Can you maybe put those stats in the right side of the status bar?

SamCarlberg · 2016-07-07T19:30:48Z

I think it makes more sense on the left side, that stuff is all related and it makes sense to keep them together. It's also not great for UX to put it on the right side

AustinShalit · 2016-07-07T20:37:28Z

ui/src/main/java/edu/wpi/grip/ui/pipeline/StepController.java

  @FXML
  private Labeled title;
  @FXML
+  private Label elapsedTime;


You can fix it in the fxml with padding

SamCarlberg · 2016-07-09T04:05:43Z

core/src/main/java/edu/wpi/grip/core/metrics/Analysis.java

+  // >1: error
+  private static final double alpha = 0.35;
+
+  private final ExponentialMovingAverage ema = new ExponentialMovingAverage(alpha);


Maybe change this to a normal arithmetic moving average, the output of this still isn't very consistent.

AustinShalit · 2016-07-10T08:30:52Z

When I open the analysis window I get a stack overflow: pastebin

Looks like it happens when I click and drag the window.

SamCarlberg · 2016-07-10T16:09:59Z

Does it only happen when you move the window?

AustinShalit · 2016-07-11T16:45:19Z

Here is another log and a video:

http://pastebin.com/JinS2Frq
https://youtu.be/BcppMd6XX-Q

SamCarlberg · 2016-07-11T17:01:45Z

That looks like a JavaFX bug to me

JLLeitschuh · 2016-07-11T19:42:04Z

core/src/main/java/edu/wpi/grip/core/events/TimerEvent.java

+    checkNotNull(data, "data");
+    this.source = source;
+    this.elapsedTime = elapsedTime;
+    this.data = data;


checkNotNull(source, "source"); returns the source so you can just use it in the same line as the assignment.

This is cleaner IMO

SamCarlberg · 2016-08-04T16:56:21Z

Can you add the column with the results from the last benchmark??

I think I'll make it CSV in a TextArea to make it easy to import into stuff like excel and google sheets

Fixed an issue with steps not being in their proper order in the analysis

JLLeitschuh

The places that I'm not totally clear what's going on could use some inline documentation.
I'd rather avoid using instanceof if possible. Can you try to avoid that.

JLLeitschuh · 2016-09-15T01:21:44Z

core/src/main/java/edu/wpi/grip/core/StepIndexer.java

+   * @param o2 the second step to compare
+   */
+  @Override
+  default int compare(Step o1, Step o2) {


Interesting solution.

JLLeitschuh · 2016-09-15T01:28:19Z

core/src/main/java/edu/wpi/grip/core/metrics/Timer.java

+      started();
+      target.run();
+    } finally {
+      stopped();


Do you want to catch any potential exceptions this may throw?

No, exceptions are propagated up the stack and are caught.

JLLeitschuh · 2016-09-15T01:29:30Z

core/src/main/java/edu/wpi/grip/core/metrics/Timer.java

+   * @throws IllegalStateException if this a call to this method is not preceded by a call to
+   *                               {@link #started()}.
+   */
+  public synchronized void stopped() {


Should this be renamed stop?
Its an action, stopped infers a question.

JLLeitschuh · 2016-09-15T01:34:04Z

ui/src/main/java/edu/wpi/grip/ui/MainWindowController.java

+      analysisStage.setOnCloseRequest(event -> eventBus.post(BenchmarkEvent.finished()));
+      eventBus.register(controller);
+      analysisStage.showAndWait();
+      eventBus.unregister(controller); // unregister to avoid memory leak


How does this work on a high DPI screen?
The fonts get all weird on those sort of screens.

JLLeitschuh · 2016-09-15T01:36:19Z

ui/src/main/java/edu/wpi/grip/ui/analysis/AnalysisWindowController.java

+      sb.append(stddev.get(i));
+      sb.append('\n');
+    }
+    return sb.toString();


Are you sure this is correctly formatted CSV?
This isn't using a library to create CSV and there aren't any tests for this code.

I have a plugin from codecov that shows me in the PR what code is covered and what isn't.
This isn't covered by tests.
https://chrome.google.com/webstore/detail/codecov-extension/keefkhehidemnokodkdkejapdgfjmijf/related

JLLeitschuh · 2016-09-15T01:40:18Z

ui/src/main/java/edu/wpi/grip/ui/pipeline/StepController.java

  }

+  @Subscribe
+  @SuppressWarnings("PMD.UnusedPrivateMethod")


There must be a way to disable this error when there is this annotation on the method.
Not sure how to do it.

JLLeitschuh · 2016-09-15T01:41:23Z

ui/src/main/java/edu/wpi/grip/ui/analysis/AnalysisWindowController.java

+        final String formatStyle = "-fx-accent: hsb(%.2f, 100%%, 100%%);";
+        progressBar.setStyle(String.format(formatStyle, h));
+      } else {
+        progressBar.setStyle("-fx-accent: hsb(180, 100%, 75%);"); // blue-gray


Want to use css classes to do this?

JLLeitschuh · 2016-09-15T01:42:00Z

ui/src/main/java/edu/wpi/grip/ui/analysis/AnalysisWindowController.java

+    table.setItems(tableItems);
+
+    benchmarkRunsField.textProperty().addListener((observable, oldValue, newValue) -> {
+      if ((oldValue.isEmpty() && "0".equals(newValue)) || !newValue.matches("[\\d]*")) {


What is this regex checking?

Checks if the input is a number

JLLeitschuh · 2016-09-15T01:43:37Z

ui/src/main/java/edu/wpi/grip/ui/MainWindowController.java

+  @Subscribe
+  @SuppressWarnings({"PMD.UnusedPrivateMethod", "PMD.UnusedFormalParameter"})
+  private void runStopped(TimerEvent event) {
+    if (!(event.getTarget() instanceof PipelineRunner)) {


instanceof is bad practice. What are you trying to do here?
Can you do something different here?

JLLeitschuh · 2016-09-15T01:44:19Z

ui/src/main/java/edu/wpi/grip/ui/analysis/AnalysisWindowController.java

+  @Subscribe
+  @SuppressWarnings("PMD.UnusedPrivateMethod")
+  private void onRun(TimerEvent event) {
+    if (event.getTarget() instanceof Step) {


Can you avoid this instance of check?

Why do you need to do this instanceof check?

JLLeitschuh · 2016-10-06T17:48:17Z

core/src/main/java/edu/wpi/grip/core/metrics/Statistics.java

+  /**
+   * Gets the number of samples.
+   */
+  public int getN() {


Can this be something like sampleCount or number or dataPoints? getN seems ambiguous.

JLLeitschuh

Just a few things.
Also, can you do something other than that instanceof check?

JLLeitschuh · 2016-10-06T17:49:09Z

core/src/main/java/edu/wpi/grip/core/metrics/Statistics.java

+  private final int n;
+  private final double sum;
+  private final double mean;
+  private final double s;


Can you rename this to something like sqrt?

It's the standard deviation, s is a standard symbol

JLLeitschuh · 2016-10-06T17:50:14Z

core/src/main/java/edu/wpi/grip/core/metrics/Timer.java

+ * </code></pre>
+ * </p>
+ */
+public class Timer {


Annotate with @ThreadSafe?

JLLeitschuh · 2016-10-06T17:52:41Z

core/src/main/java/edu/wpi/grip/core/metrics/Statistics.java

+   * @param value the value to calculate the hotness of
+   * @return the hotness of the given value.
+   */
+  public double hotness(double value) {


What is the range of hotness values? 0-10? 0 to 1? This should be documented.

Hotness can be any number >= 0. The current implementation is just the number of standard deviations above the mean

How do you represent hotness on the UI? What is the scale? What is the max value of the UI?

Hotness is represented by the hue of the bar. The length of the bar represents what percentage of time the step takes. Hotness values over three standard deviations are clamped to avoid having high-sigma steps having the same color as low-sigma steps.

Can you document that a little bit better in the javadoc?

Imagine I don't understand statistics and want to use this class. :p

Conflicts: ui/src/main/java/edu/wpi/grip/ui/MainWindowController.java

JLLeitschuh · 2016-10-31T17:00:24Z

LGTM!

![Approved with PullApprove](https://img.shields.io/badge/GRIP Merge Approvers-approved-brightgreen.svg)

Add support for timing steps and pipelines

3470404

SamCarlberg reviewed Jul 7, 2016
View reviewed changes

SamCarlberg added 2 commits July 7, 2016 11:40

Suppress PMD warnings

72a02ee

Add documentation to StepStarted and StepFinishedEvents

0a9302c

AustinShalit reviewed Jul 7, 2016
View reviewed changes

Add analysis of step timings

bbf2379

SamCarlberg reviewed Jul 9, 2016
View reviewed changes

SamCarlberg added 4 commits July 9, 2016 15:24

Updated UI test

14911e5

Add benchmarking to analysis window

0a53a78

Undo accidental formatting

b71d79a

Fix and suppress issues from Codacy

493560f

SamCarlberg force-pushed the feat/timing branch from 5433314 to 493560f Compare July 10, 2016 05:27

SamCarlberg added 2 commits July 10, 2016 17:09

Add tests for Timer

eed8a26

Improve UI and increase tolerance for timer tests

0d8a8a8

SamCarlberg force-pushed the feat/timing branch from 34d334c to 0d8a8a8 Compare July 11, 2016 14:18

SamCarlberg added 2 commits July 11, 2016 10:42

Increase timer test tolerance to 5ms (5%)

849ad71

Add tests for BenchmarkRunner

b0dca11

JLLeitschuh reviewed Jul 11, 2016
View reviewed changes

Replace custom FixedSizeQueue with guava's EvictingQueue

785ff04

AustinShalit modified the milestones: v1.5.0, 1.6.0 Aug 27, 2016

SamCarlberg added 4 commits August 30, 2016 13:50

Add option to export results to CSV.

8b7218d

Fixed an issue with steps not being in their proper order in the analysis

Merge branch master into feat/timing

5e525ca

Fix tests

a513c84

Add tests for metrics

9dd68f4

JLLeitschuh requested changes Sep 15, 2016

View reviewed changes

SamCarlberg added 3 commits October 4, 2016 10:56

Move CSV to it's own class

b2f9d4e

Merge master

a0aaaeb

Fix some checkstyle stuff I missed

6e06b3b

AustinShalit modified the milestones: 1.6.0, v1.5.0 Oct 5, 2016

Rename Timer.stopped() to stop()

7c3b79d

JLLeitschuh reviewed Oct 6, 2016

View reviewed changes

JLLeitschuh requested changes Oct 6, 2016

View reviewed changes

Delete redudant Analysis class and improve variable names in Statistics

3d9f2b6

SamCarlberg force-pushed the feat/timing branch from f7776c9 to 3d9f2b6 Compare October 11, 2016 15:45

SamCarlberg added 4 commits October 11, 2016 14:25

Improve Javadoc for statistics hotness function

b01b2b5

Fix import order

b61da85

Merge remote-tracking branch 'refs/remotes/WPIRoboicsProjects/master'

0c3b769

Conflicts: ui/src/main/java/edu/wpi/grip/ui/MainWindowController.java

Move analysis window to fx:include

3d7481c

JLLeitschuh approved these changes Oct 31, 2016

View reviewed changes

SamCarlberg merged commit 31e6b93 into WPIRoboticsProjects:master Oct 31, 2016

Comments

Conversation

SamCarlberg commented Jul 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SamCarlberg commented Jul 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jul 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 54.25% (diff: 51.56%)

Uh oh!

JLLeitschuh commented Jul 7, 2016

Uh oh!

SamCarlberg commented Jul 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AustinShalit commented Jul 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SamCarlberg commented Jul 10, 2016

Uh oh!

AustinShalit commented Jul 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SamCarlberg commented Jul 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SamCarlberg commented Aug 4, 2016

Uh oh!

JLLeitschuh 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JLLeitschuh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

SamCarlberg commented Jul 7, 2016 •

edited

Loading

SamCarlberg commented Jul 7, 2016 •

edited

Loading

codecov-io commented Jul 7, 2016 •

edited

Loading

SamCarlberg commented Jul 7, 2016 •

edited

Loading

AustinShalit commented Jul 10, 2016 •

edited

Loading

AustinShalit commented Jul 11, 2016 •

edited

Loading

JLLeitschuh Oct 6, 2016 •

edited

Loading

JLLeitschuh commented Oct 31, 2016 •

edited

Loading