Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

benchmark remotion #1358

Merged
merged 28 commits into from Oct 7, 2022
Merged

benchmark remotion #1358

merged 28 commits into from Oct 7, 2022

Conversation

uragirii
Copy link
Contributor

@uragirii uragirii commented Oct 2, 2022 •
edited by issuehunt-oss bot

Fixes #1250

The repo used for benchmarking is created here https://github.com/uragirii/remotion-bench. We can merge this repo in the remotion-dev org to centralise the tests.

The current API for the command is following:

npx remotion benchmark ./src/index --codec=h264 --runs=5 --compositions=Main,Canvas,CSS --concurrencies=2,3

I've also added docs page for benchmark command.

Pending Tasks:

  • Make progress bar to show progress of test and render times
    - [ ] Download benchmark repository. (run git clone)
    - [ ] Save assets in public folder rather using URL for referring them as network speed can cause inconsistent tests
IssueHunt Summary

Referenced issues

This pull request has been submitted to:

@vercel
Copy link

vercel bot commented Oct 2, 2022 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
remotion ✅ Ready (Inspect) Visit Preview Oct 7, 2022 at 3:11PM (UTC)

@uragirii uragirii marked this pull request as draft October 2, 2022 09:41
JonnyBurger
JonnyBurger reviewed Oct 3, 2022
View reviewed changes
packages/cli/src/benchmark.ts Outdated

const DEFUALT_RUNS = 3;
const DEFAULT_COMP_ID = 'Main';
const DEFAULT_FILE_PATH = './src/index';
Copy link
Member

Choose a reason for hiding this comment

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

Let's make the user explicitly pass the path: npx remotion benchmark src/index.tsx ... for now.
We will tackle this in another issue where this can be omitted for all commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we don't need to download (or clone) benchmark repo?

Copy link
Member

Choose a reason for hiding this comment

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

No, it can be a entry point, URL, or filepath to a bundle (Through #1048 in the future)

@JonnyBurger
Copy link
Member

This looks pretty good! Have not yet cloned and played around with it, but I agree with the design decisions taken here! :)
Will take a look again later or when you mark it as ready.

@uragirii
Copy link
Contributor Author

uragirii commented Oct 3, 2022

This looks pretty good! Have not yet cloned and played around with it, but I agree with the design decisions taken here! :) Will take a look again later or when you mark it as ready.

Thanks for looking into it. I also found out about hyperfine. Do you think printing the output like this (mean and standard deviation along with "Benchmark #1" etc) would be much nicer?

@JonnyBurger
Copy link
Member

@uragirii Love hyperfine and how they output the stuff! That would be super cool!

@uragirii uragirii changed the title wip: benchmark remotion benchmark remotion Oct 4, 2022
@uragirii uragirii marked this pull request as ready for review October 4, 2022 18:05
@JonnyBurger
Copy link
Member

Ah, I had some pending comments that did not get submitted, will send them now! And push it forward as well

JonnyBurger
JonnyBurger requested changes Oct 7, 2022
View reviewed changes
packages/cli/src/benchmark.ts Outdated Show resolved Hide resolved
packages/cli/src/benchmark.ts Outdated Show resolved Hide resolved
packages/cli/src/benchmark.ts Outdated Show resolved Hide resolved
packages/cli/src/benchmark.ts Outdated

const DEFUALT_RUNS = 3;
const DEFAULT_COMP_ID = 'Main';
const DEFAULT_FILE_PATH = './src/index';
Copy link
Member

Choose a reason for hiding this comment

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

No, it can be a entry point, URL, or filepath to a bundle (Through #1048 in the future)

packages/cli/src/benchmark.ts Outdated Show resolved Hide resolved
@JonnyBurger
Copy link
Member

Implemented the changes I suggested and also updated the autocomplete files for Fig.io! 😁 I am now happy to ship this.

@uragirii
Copy link
Contributor Author

uragirii commented Oct 7, 2022 •
edited

Ah you beat me to it. Sorry I was busy at work couldn't implement them. When will benchmark command ship (in minor or patch)? Just checked the docs available in next patch version.

@JonnyBurger
Copy link
Member

@uragirii Planning to ship it today!
You know the question, for statistics we would like to know - how long did it approximately take you to solve the issue?

@uragirii
Copy link
Contributor Author

uragirii commented Oct 7, 2022

how long did it approximately take you to solve the issue?

Its a tough one for this PR but the MVP was ready under 1hr. Most time went into formatting the output correctly/validating. Overall i would say 2-3hrs atleast.

@JonnyBurger JonnyBurger merged commit 81f6ed2 into remotion-dev:main Oct 7, 2022
@JonnyBurger
Copy link
Member

@uragirii! Thanks! I'll post the overall statistics at the end of October 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JonnyBurger JonnyBurger JonnyBurger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce a way to benchmark a computer with Remotion
2 participants
@uragirii @JonnyBurger