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

test(benchmarks): add criterion benchmarks for paragraph #262

Merged
merged 1 commit into from
Jun 16, 2023

Conversation

joshka
Copy link
Member

@joshka joshka commented Jun 15, 2023

To run the benchmarks:

cargo bench

And then open the generated target/criterion/report/index.html in a
browser.

@joshka joshka marked this pull request as draft June 15, 2023 04:37
@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #262 (5eb1163) into main (0bf6af1) will not change coverage.
The diff coverage is n/a.

❗ Current head 5eb1163 differs from pull request most recent head e1aca73. Consider uploading reports for the commit e1aca73 to get more accurate results

@@           Coverage Diff           @@
##             main     #262   +/-   ##
=======================================
  Coverage   81.76%   81.76%           
=======================================
  Files          34       34           
  Lines        6591     6591           
=======================================
  Hits         5389     5389           
  Misses       1202     1202           

@joshka joshka marked this pull request as ready for review June 15, 2023 08:50
@joshka
Copy link
Member Author

joshka commented Jun 15, 2023

image image

There are many other graphs and stats allowing comparisons between each method with the same number of lines, or with each number of lines with the same method.

image image

@joshka
Copy link
Member Author

joshka commented Jun 15, 2023

Note that the lint on this is related to pulling in fakeit which pulls in an old version of the time crate via chrono. I've submitted a PR to fix the issue in fakeit PumpkinSeed/fakeit#20 and checked that none of the affected code mentioned in the rustsec advisory are actually called from fakeit, so this is not a blocker (I think at least - assuming that dev-dependencies don't trigger upstream issues).

The cargo deny issue is resolved by the PR.

❯ git d
Found existing alias for "git". You should use: "g"
diff --git a/Cargo.toml b/Cargo.toml
index f17dada..ea6923f 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -53,6 +53,9 @@ argh = "0.1"
 criterion = { version = "0.5", features = ["html_reports"] }
 fakeit = "1.1"

+[patch.crates-io]
+fakeit = { git = "https://github.com/joshka/fakeit.git" }
+
 [[bench]]
 name = "paragraph"
 harness = false

❯ cargo deny check
warning[source-not-allowed]: detected 'git' source not explicitly allowed
   ┌─ /Users/joshka/local/ratatui/Cargo.lock:36:14
   │
36 │ fakeit 1.1.1 git+https://github.com/joshka/fakeit#329e2393c3aee59c1555c2789480830b0c5cc38d
   │              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ source
   │
   = fakeit v1.1.1
     └── (dev) ratatui v0.21.0

advisories ok, bans ok, licenses ok, sources ok

benches/paragraph.rs Outdated Show resolved Hide resolved
@joshka joshka enabled auto-merge June 16, 2023 07:56
To run the benchmarks:

    cargo bench

And then open the generated `target/criterion/report/index.html` in a
browser.

- add the BSD 2 clause and ISC licenses to the `cargo deny` allowed
licenses list (as a transitive dependency of the `fakeit` crate).
- remove the WTFPL license from the `cargo deny` allowed licenses list
as it is unused and causes a warning when running the check.
@joshka
Copy link
Member Author

joshka commented Jun 16, 2023

Fixed the lint issue in deny.toml by adding compatible licenses (BSD 2 clause and ISC) and removing WTFPL.
Fakeit CVE is fixed (and released):

Copy link
Sponsor Member

@orhun orhun left a comment

Choose a reason for hiding this comment

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

LGTM!

@joshka joshka added this pull request to the merge queue Jun 16, 2023
Merged via the queue into ratatui-org:main with commit 6c2fbbf Jun 16, 2023
9 of 10 checks passed
samyosm pushed a commit to samyosm/ratatui that referenced this pull request Jun 18, 2023
To run the benchmarks:

    cargo bench

And then open the generated `target/criterion/report/index.html` in a
browser.

- add the BSD 2 clause and ISC licenses to the `cargo deny` allowed
licenses list (as a transitive dependency of the `fakeit` crate).
- remove the WTFPL license from the `cargo deny` allowed licenses list
as it is unused and causes a warning when running the check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants