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

url: faster encodePathChars function. #43566

Closed
wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

This is roughly twice as fast as before.

Signed-off-by: Ruben Bridgewater [email protected]

@BridgeAR BridgeAR requested a review from aduh95 June 25, 2022 11:21
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jun 25, 2022
This is roughly twice as fast as before.

Signed-off-by: Ruben Bridgewater <[email protected]>
@BridgeAR BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 25, 2022
@aduh95
Copy link
Contributor

aduh95 commented Jun 25, 2022

Hum it looks like we don't have benchmarks for fileURLToPath. Did you use a script to test the perf improvements?

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 25, 2022
@nodejs-github-bot
Copy link
Collaborator

@BridgeAR
Copy link
Member Author

I ran a simple benchmark locally:

const cwd = process.cwd();
console.time(); for (let i = 0; i < 1e7; i++) encodePathChars(cwd); console.timeEnd()

I sometimes replaced cwd with different values. After looking at it again, it's not faster anymore after I simplified the implementation (using an object instead of an array). I might have missed running the benchmark again. It is probably not really worth the more difficult implementation, so I'm closing this again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants