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.

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

Added benchmark suite #2153

Merged
merged 27 commits into from Aug 16, 2021
Merged

Added benchmark suite #2153

merged 27 commits into from Aug 16, 2021

Conversation

RunDevelopment
Copy link
Member

@RunDevelopment RunDevelopment commented Jan 4, 2020

This adds a benchmark suite to Prism.

Since I already explained everything in benchmark.html, I won't repeat myself but the basic idea is that the whole benchmark configuration is in benchmark/config.js.

Points to discuss

  • I added support for multiple remote branches because it was easy and useful for testing my implementation but maybe we don't need it? It was pretty easy to implement, so let's just keep it.
  • rn, I only display the mean, margin of error, and sample count. Anything else we need? More information can be added in future PRs if needed.
  • The summary is quite simple, so maybe we could add something? It displays enough information IMO.

benchmark/files/prism.js Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Dec 28, 2020

No JS Changes

Generated by 🚫 dangerJS against 9db6918

Copy link
Collaborator

@JaKXz JaKXz left a comment

Choose a reason for hiding this comment

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

This looks really interesting! What would it take to get across the line? Could I help out in some way?

@RunDevelopment
Copy link
Member Author

I think this just needs a proper review.

@mAAdhaTTah I looked over it again and did some minor refactoring. I think it's good to go but it would be nice if you could review it.

Copy link
Collaborator

@JaKXz JaKXz left a comment

Choose a reason for hiding this comment

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

FWIW I think it looks good 👍🏽

benchmark/benchmark.js Outdated Show resolved Hide resolved
@JaKXz
Copy link
Collaborator

JaKXz commented Aug 16, 2021

@RunDevelopment I think this can be merged :)

@RunDevelopment
Copy link
Member Author

There's still one open comment.

cc @mAAdhaTTah

@mAAdhaTTah
Copy link
Member

@RunDevelopment sorry, I approved, I don't think that change is necessary.

@RunDevelopment
Copy link
Member Author

@mAAdhaTTah Ahh ok. I always read "approved + comments" as "approved if all comments resolved". I'll keep that in mind in the future.

@RunDevelopment RunDevelopment merged commit 44456b2 into PrismJS:master Aug 16, 2021
@RunDevelopment RunDevelopment deleted the benchmark branch August 16, 2021 18:08
@mAAdhaTTah
Copy link
Member

In this case, I thought I had commented and resolved so that's my bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants