Skip to content

Setup basic perf CI #417

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

Merged
merged 17 commits into from
Jan 28, 2025
Merged

Setup basic perf CI #417

merged 17 commits into from
Jan 28, 2025

Conversation

atuchin-m
Copy link
Collaborator

The PR:

  1. adds a CI job to estimate perf via github-action-benchmark that fails on +30% regression. The job also pushes the data to GH pages to make graphs.
    2 updates easylists*, remove file duplicates and add brave main list in the benchmarks. Also provides a script to do the next updates.
  2. adds a benchmark to measure the time to check the first request (~20ms).

An example (from a forked repo):
image
image

@atuchin-m atuchin-m self-assigned this Jan 24, 2025
const readline = require("readline");
const fs = require("fs");

const rl = readline.createInterface({
Copy link
Collaborator

Choose a reason for hiding this comment

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

wouldn't it be easier to pass the key as a command parameter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

make sense, done.

);

try {
execSync("unzip extension.zip list.txt");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd either do this part in a temp directory or warn if there are any existing files that could get overwritten

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to use a temp dir.

Comment on lines +17 to +19

[features]
default-panic-hook = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this does anything?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

One of the changes in Rust 1.84 is rust-lang/rust#132577, so this is now a lint warning (and thus an error since we've configured lint warnings as errors). I think it's resolved in newer versions of Neon, but we can leave it here as-is for now.

@atuchin-m atuchin-m merged commit 61d339e into master Jan 28, 2025
8 checks passed
@atuchin-m atuchin-m deleted the setup-basic-perf-ci branch January 28, 2025 18:26
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.

5 participants