search

jshttp / etag

Create simple HTTP ETags
MIT License
256 stars 32 forks source link

perf: use substring instead of replace #20

Closed billouboq closed 7 years ago

billouboq commented 7 years ago

I saw a 10% improvement in bench with this change.

dougwilson commented 7 years ago

Nice @billouboq ! I'm wondering about the following, if you can help me out:

1) The code used to remove all the equal signs at the end of the string, but this change only ever will remove a single one. Is that an intentional change? 2) The PR is failing because one of your code branches is untested. Please add a test for all the branch conditions of your code. 3) Would you mind sharing the benchmark code you used? Or are you just looking at npm run bench? Sometimes these types of changes can speed up one version of Node.js but hurt others, so we want to make sure the change is neutral to positive on all versions of Node.js this module supports.

billouboq commented 7 years ago

The code used to remove all the equal signs at the end of the string, but this change only ever will remove a single one. Is that an intentional change?

My bad, fixed it in the last commit.

The PR is failing because one of your code branches is untested. Please add a test for all the branch conditions of your code.

Sorry, Not sure what you mean by that.

Would you mind sharing the benchmark code you used? Or are you just looking at npm run bench? Sometimes these types of changes can speed up one version of Node.js but hurt others, so we want to make sure the change is neutral to positive on all versions of Node.js this module supports.

I was just looking at npm run bench, but will try with older version of nodejs as well.

billouboq commented 7 years ago

Following the benchmark I saw :

dougwilson commented 7 years ago

Re the failing PR: the change of the code changed the flow and the coverage is back to 100%. Thanks for this, and glad to see that you're able to see the boost in our already existing bench code :) !