Skip to content
This repository has been archived by the owner on Aug 20, 2024. It is now read-only.

Add basic transform benchmarking infrastructure and speed up Resolve Kinds #1475

Merged
merged 4 commits into from
May 14, 2020

Conversation

jackkoenig
Copy link
Contributor

@jackkoenig jackkoenig commented Mar 25, 2020

This is the first in a larger effort I'm working on to eliminate low hanging fruit performance issues. This is primarily a proof of concept and to show that even in seemingly simple passes we have pretty obvious room for performance improvement.

Resolve Kinds Benchmarking

I would appreciate someone checking my numbers, perhaps using BOOM or some large design from Chipyard.

These numbers were collected by running sbt benchmark:assembly then running using the fat jar.

$ java -version
openjdk version "1.8.0_232"
OpenJDK Runtime Environment (build 1.8.0_232-8u232-b09-0ubuntu1~16.04.1-b09)
OpenJDK 64-Bit Server VM (build 25.232-b09, mixed mode)
$ java -Xmx8G utils/bin/firrtl-benchmark.jar firrtl.benchmark.hot.ResolveKindsBenchmark <large_sifive_design>.pb 10 10

Times in milliseconds

Commit Desc Mean Median Stddev
e5ca9ac no change 571 571 49
567b271 use HashMap 342 341 20
5148c7f eliminate traversal 267 256 22

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you state the API impact?
  • Did you specify the code generation impact?
  • Did you request a desired merge strategy?

Type of Improvement

  • performance improvement

API Impact

Technically this changes some APIs in ResolveKinds. This isn't backportable so maybe I should do this by copying the code and modifying the copy then deprecating the old ResolveKinds?

Backend Code Generation Impact

None

Desired Merge Strategy

  • Rebase: You will rebase the PR onto master and it will be merged with a merge commit.

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (1.2.x, 1.3.0, 1.4.0) ?
  • Did you review?
  • Did you mark as Please Merge?

@jackkoenig jackkoenig requested a review from a team as a code owner March 25, 2020 03:36
@jackkoenig jackkoenig force-pushed the transform-benchmarking-resolve-kinds branch 2 times, most recently from 69e4d06 to 5148c7f Compare May 7, 2020 05:37
@jackkoenig jackkoenig mentioned this pull request May 9, 2020
14 tasks
@albert-magyar albert-magyar force-pushed the transform-benchmarking-resolve-kinds branch from 5148c7f to 15134df Compare May 14, 2020 20:20
Copy link
Contributor

@albert-magyar albert-magyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jackkoenig, I rebased this past the split of the Resolves file and made the find_port API change part of a separate commit so only that commit and the type alias change need to be modified in cherry-pick.

I tried this out and I really like it. I'm assuming you relied on this for #1582 and the like?

@albert-magyar albert-magyar self-requested a review May 14, 2020 20:25
@albert-magyar
Copy link
Contributor

This is the first in a larger effort I'm working on to eliminate low hanging fruit performance issues. This is primarily a proof of concept and to show that even in seemingly simple passes we have pretty obvious room for performance improvement.

These numbers were collected by running sbt benchmark:assembly then running using the fat jar.

I think you mean sbt benchmark/assembly since it's a separate project, not a config of firrtl.

@albert-magyar albert-magyar self-requested a review May 14, 2020 20:39
Copy link
Contributor

@albert-magyar albert-magyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the silly ping-pong on the reviews, but I just wanted to make sure I had the usage instructions right before final approval. I tested it out myself but neglected to follow up and comment on this.

I think your code snippet from the top may be out of date (sbt / vs. : and missing -cp flag for java). I imagine that going forward (when there will be multiple benchmarks apps), the right way to run this will be:

sbt benchmark/assembly
java -Xmx8G -cp utils/bin/firrtl-benchmark.jar firrtl.benchmark.hot.ResolveKindsBenchmark <design> 10 10

@jackkoenig
Copy link
Contributor Author

I tried this out and I really like it. I'm assuming you relied on this for #1582 and the like?

Yep, it's been very useful. I also have some JMH stuff that I want to add, although it is not great for benchmarking anything that runs very long. A good tool to have in the toolbox nonetheless.

I think your code snippet from the top may be out of date (sbt / vs. : and missing -cp flag for java).

I think you're right, my bad

I'll note that this PR is not backportable because it changes the API of `ResolveKinds'. I redid it locally to make it backportable and I can either force push over this PR or push to a new branch.

@albert-magyar
Copy link
Contributor

I'll note that this PR is not backportable because it changes the API of `ResolveKinds'. I redid it locally to make it backportable and I can either force push over this PR or push to a new branch.

Resolution: we'll just keep it the desired way for 1.4 and beyond, and we'll modify the PR for backportability.

@albert-magyar albert-magyar added this to the 1.3.x milestone May 14, 2020
@albert-magyar albert-magyar merged commit 1705980 into master May 14, 2020
@albert-magyar albert-magyar deleted the transform-benchmarking-resolve-kinds branch May 14, 2020 21:23
@mergify mergify bot added the Backported This PR has been backported to marked stable branch label May 14, 2020
mergify bot added a commit that referenced this pull request May 14, 2020
…Kinds (bp #1475) (#1622)

* Add benchmark for ResolveKinds with hot JIT

* Use HashMap instead of LinkedHashMap in ResolveKinds

* Modify to deprecate old methods for backport

(cherry picked from commit 0f78e2d)

Co-authored-by: Albert Magyar <albert.magyar@gmail.com>

* Eliminate unnecessary traversals in ResolveKinds

* Modify for backporting

* Make find_port return Unit and use Foreachers in ResolveKinds

* Modify for backporting

Co-authored-by: Jack Koenig <koenig@sifive.com>
Co-authored-by: Albert Magyar <albert.magyar@gmail.com>
mergify bot pushed a commit that referenced this pull request May 15, 2020
* Use HashMap instead of LinkedHashMap in ResolveKinds

* Backport to 1.2.x

(cherry picked from commit 0f78e2d)

Co-authored-by: Albert Magyar <albert.magyar@gmail.com>

* Eliminate unnecessary traversals in ResolveKinds

* Modify for backporting

* Make find_port return Unit and use Foreachers in ResolveKinds

* Modify for backporting

Co-authored-by: Jack Koenig <koenig@sifive.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Backported This PR has been backported to marked stable branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants