Skip to content

Dynamo View Performance Benchmarks#9623

Merged
QilongTang merged 13 commits intomasterfrom
PerformanceBenchmark
Apr 8, 2019
Merged

Dynamo View Performance Benchmarks#9623
QilongTang merged 13 commits intomasterfrom
PerformanceBenchmark

Conversation

@QilongTang
Copy link
Contributor

@QilongTang QilongTang commented Apr 4, 2019 •
edited
Loading

Please Note:

  1. Before submitting the PR, please review How to Contribute to Dynamo
  2. Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
  3. PRs will be reviewed from oldest to newest
  4. If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
  5. PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
  6. PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
  7. PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

This PR:

  1. Introduce PerformanceTestHelper
  2. Introduce DynamoView Performance Test Base
  3. Re-organize the code base and add more comments to help understanding
  4. Updated the console app to default to run both Model and Viewl test cases
  5. Add FastBenchmarkReleaseConfig as default performance benchmark job. It contains DynamoMaxWarmuoCount = 9, DynamoMaxIterationCount = 9 to avoid the case, the benchmark sometimes runs forever and reach to the 100 times internal limit.
  6. Add DebugInProcessConfig as an alternative config that dev can apply locally for debugging benchmark running

image

Time consumption running view benchmarks with the current suite:
image

Time consumption running model benchmarks with the current suite:
image

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@ColinDayOrg @scottmitchell @aparajit-pratap

FYIs

@DynamoDS/dynamo

@QilongTang
Initial Commit
20640ae
@QilongTang
Add interface for performance tests
fe395f5
@QilongTang
Merge branch 'master' into PerformanceBenchmark
ac4a789
@QilongTang
Separate Performance Tests into reasonable pieces
309fe6e 
1. introduce helper for grabbing config
2. introduce a second performance test case
3. make the console app entrance cleaner
@QilongTang
Clean Up
993dd79
@QilongTang QilongTang added the WIP label Apr 4, 2019
@QilongTang
Copy link
Contributor Author

QilongTang commented Apr 5, 2019

This PR is ready for review in terms of getting view execution time, I will investigate at how to get render time

@QilongTang QilongTang added PTAL Please Take A Look 👀 and removed WIP labels Apr 5, 2019
@QilongTang QilongTang changed the title [WIP] Performance Benchmark Part II Performance Benchmark Part II Apr 5, 2019
aparajit-pratap
aparajit-pratap requested changes Apr 8, 2019
View reviewed changes
ColinDayOrg
ColinDayOrg reviewed Apr 8, 2019
View reviewed changes
ColinDayOrg
ColinDayOrg reviewed Apr 8, 2019
View reviewed changes
ColinDayOrg
ColinDayOrg reviewed Apr 8, 2019
View reviewed changes
ColinDayOrg
ColinDayOrg reviewed Apr 8, 2019
View reviewed changes
@ColinDayOrg
Copy link
Contributor

ColinDayOrg commented Apr 8, 2019

Other than a couple of comments about documentation and the empty class, LGTM.

@QilongTang
Copy link
Contributor Author

QilongTang commented Apr 8, 2019

Addressed comments, please take a look @ColinDayOrg @aparajit-pratap

@aparajit-pratap
Copy link
Contributor

aparajit-pratap commented Apr 8, 2019

@QilongTang just one last question about the reasoning for selection of the fast config settings for iteration count, warmup count, etc. Is there some heuristic you used to come up with these numbers?

@ColinDayOrg
Copy link
Contributor

ColinDayOrg commented Apr 8, 2019

LGTM :shipit:

@QilongTang
Copy link
Contributor Author

QilongTang commented Apr 8, 2019

@aparajit-pratap @ColinDayOrg These numbers are not fixed, we should tune those. I decided these as first version because these numbers make sure the graph run with view along can be finished within an hour, the model tests finished within 15 minutes. And the number I got is quite stable and as good as before. After we hook up the benchmark with CI, we should change these numbers and decide best times to run and warmup, etc. Maybe instead of static int, we should read from some config. But more to consider in the future I believe.

@QilongTang QilongTang merged commit a356bb6 into master Apr 8, 2019
@QilongTang QilongTang deleted the PerformanceBenchmark branch April 8, 2019 22:13
@QilongTang QilongTang removed the PTAL Please Take A Look 👀 label Apr 9, 2019
@QilongTang QilongTang changed the title Performance Benchmark Part II Dynamo View Performance Benchmarks Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mjkkirschner mjkkirschner mjkkirschner left review comments

@aparajit-pratap aparajit-pratap aparajit-pratap approved these changes

+1 more reviewer

@ColinDayOrg ColinDayOrg ColinDayOrg left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@QilongTang @ColinDayOrg @aparajit-pratap @mjkkirschner