Skip to content

Conversation

dvmhmdsd
Copy link

Motivation

As discussed here #526 for prformance optimiztions, that there should be a benchmarking infrastructure:

  • Run before and after changes to measure impact
    • Execute on every Node.js version in the test matrix
    • Prevent future performance regressions
    • Provide data-driven evidence for performance claims

Copy link

socket-security bot commented Jun 20, 2025

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedobject.fromentries@​2.0.8671008151100
Addedobject.entries@​1.1.9671008552100
Addedstring.prototype.repeat@​1.0.0671007952100
Addedarray.from@​1.1.6571009353100
Addedarray-includes@​3.1.96610010055100
Addedcolors@​=1.4.010010010077100
Addedbenchmark@​2.1.41001009879100
Addedes6-shim@​0.35.810010010079100

View full report

Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

the root package.json's publishConfig.ignore will need "benchmark" added in package.json as well.

Also, can we add a GHA workflow that runs these on every push/PR, and fails if there's a big enough slowdown?

@dvmhmdsd
Copy link
Author

I added most of the comments, will find a way for the GHA @ljharb

Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

this seems to be only running benchmarks in node 20.

@dvmhmdsd
Copy link
Author

this seems to be only running benchmarks in node 20.

what versions should it run on ?

@ljharb
Copy link
Owner

ljharb commented Jun 21, 2025

Execute on every Node.js version in the test matrix

all of them

@dvmhmdsd
Copy link
Author

dvmhmdsd commented Jun 22, 2025

Currently, the benchmarking scripts use some modern features (e.g., Array.from, async/await) and rely on tools that are not compatible with 0.8 like Benchmark.js which I use for benchmarking, which only supports node 0.10. So, is compatibility with Node 0.8 for the benchmarking infrastructure is a hard requirement ? @ljharb

@ljharb
Copy link
Owner

ljharb commented Jun 22, 2025

I can definitely live with only going down to 0.10. https://npmjs.com/array.from should cover that one, and async/await can be done with Promises instead, which are polyfillable.

dvmhmdsd added 8 commits June 25, 2025 12:55
- Implemented parsing benchmarks for various query string formats including simple, nested, and array queries.
- Added stringifying benchmarks for different object structures and array formats.
- Created a runner to manage benchmark execution and results reporting.
- Introduced fixtures for generating test data patterns for benchmarking.
- Added functionality to compare current results against baseline benchmarks.
- Implemented options for saving current results as baseline and generating summary reports.
- Enhanced command-line interface for running benchmarks with various options.
- Deleted outdated benchmark result files to clean up the repository.
- Updated the `generateQuery` function in fixtures to handle default size parameter more gracefully.
- Added new dependencies for polyfills to support ES6+ features in older Node.js versions.
- Removed custom polyfills for Array.from, String.prototype.repeat, Object.fromEntries, and Object.entries, replacing them with well-maintained npm packages.
- Updated benchmark runner to improve code readability and maintainability.
- Adjusted benchmark commands in package.json for consistency and clarity.
- Added new benchmark result file with updated performance metrics for various parsing scenarios.
@dvmhmdsd
Copy link
Author

Hi @ljharb
Just following up on this PR — I’ve addressed all the comments and happy to make any further changes if needed. Please let me know if there's anything else blocking it from being merged. Thanks.

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.

2 participants