Add 'compare performance' view #3843

asgerf · 2024-11-27T10:34:35Z

Adds the 'Compare Performance' UI from the hackathon.

Activate via right-clicking on a query history item

Shows overview of predicate evaluation costs

Click a predicate to show its pipeline(s) and tuple counts per step

Changes since the hackathon:

The integration with DCA has been left out for now. I'm hoping @esbena will follow up on this.
Added support for predicate-renaming, which was WIP and not demoable in time for the Hackathon demo.
Some UI performance improvements: using useMemo and moved some state into subcomponents where a state change cause as much re-rendering.
Some clean up of the commit history; mainly squashing simple bugfixes into an earlier commit.

Despite my attempts at cleaning the commit history, it might actually be easiest to review as one giant diff.

Adds a dropdown with (at present) two options: sorting by delta and sorting by absolute delta.

This ensures we can use hooks after the check in the main component

A very hacky implementation that simply instantiates an empty `PerformanceOverviewScanner` as the "from" column (i.e. with all values empty). To see it in action, select a single query run in the query history and pick "Compare Performance" from the context menu. Then select the "Single run" option when prompted.

Only contains formatting changes

…eMetric

Predicate names can't be empty, but it can happen with the renaming feature added in the next commit.

aeisenberg

It looks ok to me in general. I haven't tried running this. It also seems isolated, so that if there is a problem, it is unlikely to affect other features.

There are a few questions and comments inside. Mostly, I'd like to either remove all TODOs and XXXs and replace them with a simple comment. If you think you will go back later to fix them, then please create an issue to track it.

aeisenberg · 2025-01-10T21:21:40Z

extensions/ql-vscode/package.json

+        {
+          "command": "codeQLQueryHistory.comparePerformanceWith",
+          "group": "3_queryHistory@1",
+          "when": "viewItem == rawResultsItem || viewItem == interpretedResultsItem"


Should this be limited to canary users? Probably.

Suggested change

"when": "viewItem == rawResultsItem || viewItem == interpretedResultsItem"

"when": "viewItem == rawResultsItem && config.codeQL.canary || viewItem == interpretedResultsItem && config.codeQL.canary"

aeisenberg · 2025-01-10T21:25:37Z

extensions/ql-vscode/src/compare-performance/compare-performance-view.ts

+    };
+  }
+
+  protected onPanelDispose(): void {}


Do you want to remove references to resultsView and labelProvider?

Suggested change

protected onPanelDispose(): void {}

protected onPanelDispose(): void {

this.resultsView = null;

this.labelProvider = null;

}

Hmmm...not sure what the lifecycle of the view is. Is there a single view instance created for the entire extension, or is it one view per window that's opened?

aeisenberg · 2025-01-10T21:31:34Z

extensions/ql-vscode/src/log-insights/log-scanner.ts

+  progress?: ProgressCallback,
+): Promise<T> {
+  progress?.({
+    // XXX all scans have step 1 - the backing progress tracker allows increments instead of steps - but for now we are happy with a tiny UI that says what is happening


I prefer avoiding XXX or TODOs in code that we're probably not going to get to.

Suggested change

// XXX all scans have step 1 - the backing progress tracker allows increments instead of steps - but for now we are happy with a tiny UI that says what is happening

// all scans have step 1 - the backing progress tracker allows increments instead of steps - but for now we are happy with a tiny UI that says what is happening

aeisenberg · 2025-01-10T21:33:25Z

extensions/ql-vscode/src/log-insights/performance-comparison.ts

+ */
+export interface PerformanceComparisonDataFromLog {
+  /** Names of predicates mentioned in the log */
+  names: string[];


Do we expect the lengths of these arrays to be the same for all? If not, which ones will have different sizes?

aeisenberg · 2025-01-10T21:34:39Z

extensions/ql-vscode/src/log-insights/performance-comparison.ts

+  /**
+   * List of indices into the `names` array for which we have seen a cache hit.
+   *
+   * TODO: only count cache hits prior to first evaluation?


Here and below, if the TODO is tracked in an issue, can you add the issue? If it's not tracked, can you remove the TODO?

aeisenberg · 2025-01-10T21:37:35Z

extensions/ql-vscode/src/query-history/query-history-manager.ts

+    singleItem: QueryHistoryInfo,
+    multiSelect: QueryHistoryInfo[] | undefined,
+  ) {
+    // TODO: reduce duplication with 'handleCompareWith'


Same...and below.

aeisenberg · 2025-01-10T21:38:15Z

extensions/ql-vscode/src/query-history/query-history-manager.ts

+    multiSelect: QueryHistoryInfo[] | undefined,
+  ) {
+    // TODO: reduce duplication with 'handleCompareWith'
+    multiSelect ||= [singleItem];


That's a nice shortcut. I hadn't thought of that.

aeisenberg · 2025-01-10T21:39:31Z

extensions/ql-vscode/src/query-history/query-history-manager.ts

+          ? allSelectedItems[1]
+          : allSelectedItems[0];
+      if (otherItem.databaseName !== dbName) {
+        throw new Error("Query databases must be the same.");


Minor: can you include the db names in this message?

aeisenberg · 2025-01-10T21:50:58Z

extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx

+        args.push(parseQName());
+        skipToken(",");
+      }
+      args.reverse();


Why the reverse?

aeisenberg · 2025-01-10T21:58:36Z

extensions/ql-vscode/src/view/compare-performance/RAPrettyPrinter.tsx

+        if (prefix != null) {
+          result.push(prefix.abbreviate());
+          result.push("::");
+        }


This is quite complex to me. It is combining prefix from the closure with result, a local variable. This function is complex enough that maybe VisitResult should be a class and prefix is passed in as a private field. Then this function can become a member of VisitResult.

asgerf and others added 30 commits November 27, 2024 13:47

Add Compare Performance command (WIP)

3b06977

Show evaluation and iteration counts in table

96aa770

compare-perf: Add support for sorting options

aaf23ea

Adds a dropdown with (at present) two options: sorting by delta and sorting by absolute delta.

Make RAPrettyPrinter generate JSX fragments

70ec570

Apply styling to RA predicate names

8268d68

Apply a background color to the pipeline header rows

58afeba

Make "..." clickable to reveal abbreviated name

03ca407

Also abbreviate RA names in predicate overview

317e52c

Colorize positive/negative deltas

876c5b6

Add option to choose metric

260bf0e

Check for nullness of 'data' in a separate component

453aa83

This ensures we can use hooks after the check in the main component

Simplify datasets assignment

ccf2dc6

feat: parallel log scanning

412338c

Simplify view when not in comparison mode

e039f6b

Allow word wrap to break anywhere

1d2c2cf

Use useMemo for 'nameSet'

b05ec33

Use useMemo a few places to speed up UI interactions

20f6e3d

Reformat code

558d957

Also useMemo the 'total' row computation

6568b56

Reset hasCacheMismatch when rebuilding 'rows'

6f7eb74

Reformat code again

62f3b4f

Only contains formatting changes

Use interface instead of type alias for TRow

9800fa1

Refactor OptionalValue

d008963

Split renderAbsoluteValue into renderOptionalValue and renderPredicat…

eec42c5

…eMetric

Refactor predicate row into a separate component

9a0699f

Rename TRow -> Row

48954c7

Group together rows by fingerprinting

ab00152

Factor out rendering of table body to a memoized component

4a835b8

Fix some crashes when pretty-printing an empty name

568f082

Predicate names can't be empty, but it can happen with the renaming feature added in the next commit.

asgerf force-pushed the asgerf/compare-perf-view branch 2 times, most recently from 13e78a6 to ec223fa Compare November 27, 2024 13:00

asgerf added 7 commits November 27, 2024 14:14

Add predicate-renaming support

2cae71c

Add 'per evaluation' option next to metric

d37469f

Move "total" row to the top and render the metric

8a58279

Hide "sort by" dropdown when there is no delta to sort by

b9d1551

Factor header rows into a component

afa3d55

Only warn about cache hits in comparison mode

c99bf5b

Remove unused export

aa528c6

asgerf force-pushed the asgerf/compare-perf-view branch from ec223fa to aa528c6 Compare November 27, 2024 13:14

asgerf marked this pull request as ready for review November 27, 2024 13:41

asgerf requested a review from a team as a code owner November 27, 2024 13:41

aeisenberg reviewed Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'compare performance' view #3843

Add 'compare performance' view #3843

asgerf commented Nov 27, 2024 •

edited

Loading

aeisenberg left a comment

aeisenberg Jan 10, 2025

aeisenberg Jan 10, 2025

aeisenberg Jan 10, 2025

aeisenberg Jan 10, 2025

aeisenberg Jan 10, 2025

aeisenberg Jan 10, 2025

aeisenberg Jan 10, 2025

aeisenberg Jan 10, 2025

aeisenberg Jan 10, 2025

aeisenberg Jan 10, 2025

aeisenberg Jan 10, 2025

	"when": "viewItem == rawResultsItem \|\| viewItem == interpretedResultsItem"
	"when": "viewItem == rawResultsItem && config.codeQL.canary \|\| viewItem == interpretedResultsItem && config.codeQL.canary"

	// XXX all scans have step 1 - the backing progress tracker allows increments instead of steps - but for now we are happy with a tiny UI that says what is happening
	// all scans have step 1 - the backing progress tracker allows increments instead of steps - but for now we are happy with a tiny UI that says what is happening

Add 'compare performance' view #3843

Are you sure you want to change the base?

Add 'compare performance' view #3843

Conversation

asgerf commented Nov 27, 2024 • edited Loading

Activate via right-clicking on a query history item

Shows overview of predicate evaluation costs

Click a predicate to show its pipeline(s) and tuple counts per step

Changes since the hackathon:

aeisenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asgerf commented Nov 27, 2024 •

edited

Loading