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: improve resolveObject with ObjectAssign #54092

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

EarlyRiser42
Copy link
Contributor

Summary

This PR enhances the performance of the resolveObject function by replacing the for loop used for copying properties with ObjectAssign.

Changes

Replaced the for loop in the resolveObject function with ObjectAssign and reduce to improve performance.
Based on local benchmark tests, the reduce method demonstrates a slight performance advantage over the for statement.

Benchmark Results

local benchmark shows performance improvements

                                                                confidence improvement accuracy (*)   (**)  (***)
url/url-resolve.js n=100000 path='down' href='auth'                    ***      3.76 %       ±1.32% ±1.75% ±2.28%
url/url-resolve.js n=100000 path='down' href='dot'                     ***      3.21 %       ±1.31% ±1.74% ±2.27%
url/url-resolve.js n=100000 path='down' href='file'                    ***      4.58 %       ±1.99% ±2.65% ±3.45%
url/url-resolve.js n=100000 path='down' href='idn'                     ***      4.93 %       ±1.70% ±2.26% ±2.94%
url/url-resolve.js n=100000 path='down' href='javascript'              ***      6.81 %       ±1.55% ±2.07% ±2.70%
url/url-resolve.js n=100000 path='down' href='long'                    ***      3.67 %       ±1.34% ±1.79% ±2.32%
url/url-resolve.js n=100000 path='down' href='noscheme'                ***      4.76 %       ±1.29% ±1.71% ±2.23%
url/url-resolve.js n=100000 path='down' href='percent'                 ***      3.99 %       ±1.62% ±2.16% ±2.81%
url/url-resolve.js n=100000 path='down' href='short'                   ***      4.65 %       ±0.98% ±1.31% ±1.70%
url/url-resolve.js n=100000 path='down' href='ws'                      ***      5.01 %       ±1.62% ±2.15% ±2.81%
url/url-resolve.js n=100000 path='foo/bar' href='auth'                 ***      4.06 %       ±1.47% ±1.95% ±2.54%
url/url-resolve.js n=100000 path='foo/bar' href='dot'                  ***      3.57 %       ±1.21% ±1.60% ±2.09%
url/url-resolve.js n=100000 path='foo/bar' href='file'                 ***      5.01 %       ±1.58% ±2.10% ±2.74%
url/url-resolve.js n=100000 path='foo/bar' href='idn'                  ***      4.22 %       ±1.43% ±1.90% ±2.47%
url/url-resolve.js n=100000 path='foo/bar' href='javascript'           ***      7.41 %       ±1.79% ±2.39% ±3.11%
url/url-resolve.js n=100000 path='foo/bar' href='long'                 ***      3.32 %       ±1.00% ±1.33% ±1.73%
url/url-resolve.js n=100000 path='foo/bar' href='noscheme'             ***      5.81 %       ±1.73% ±2.30% ±3.00%
url/url-resolve.js n=100000 path='foo/bar' href='percent'              ***      3.75 %       ±1.70% ±2.28% ±2.99%
url/url-resolve.js n=100000 path='foo/bar' href='short'                ***      4.49 %       ±1.53% ±2.05% ±2.68%
url/url-resolve.js n=100000 path='foo/bar' href='ws'                   ***      4.46 %       ±1.47% ±1.96% ±2.55%
url/url-resolve.js n=100000 path='sibling' href='auth'                 ***      4.02 %       ±1.32% ±1.76% ±2.30%
url/url-resolve.js n=100000 path='sibling' href='dot'                  ***      4.55 %       ±1.46% ±1.94% ±2.54%
url/url-resolve.js n=100000 path='sibling' href='file'                 ***      4.16 %       ±1.28% ±1.70% ±2.22%
url/url-resolve.js n=100000 path='sibling' href='idn'                  ***      5.04 %       ±1.38% ±1.84% ±2.39%
url/url-resolve.js n=100000 path='sibling' href='javascript'           ***      3.80 %       ±1.56% ±2.07% ±2.70%
url/url-resolve.js n=100000 path='sibling' href='long'                 ***      2.83 %       ±1.09% ±1.46% ±1.90%
url/url-resolve.js n=100000 path='sibling' href='noscheme'             ***      6.25 %       ±1.61% ±2.15% ±2.80%
url/url-resolve.js n=100000 path='sibling' href='percent'               **      4.42 %       ±2.72% ±3.62% ±4.71%
url/url-resolve.js n=100000 path='sibling' href='short'                ***      4.75 %       ±1.11% ±1.47% ±1.92%
url/url-resolve.js n=100000 path='sibling' href='ws'                   ***      3.74 %       ±1.52% ±2.02% ±2.64%
url/url-resolve.js n=100000 path='up' href='auth'                      ***      3.86 %       ±1.09% ±1.45% ±1.88%
url/url-resolve.js n=100000 path='up' href='dot'                        **      3.02 %       ±1.83% ±2.45% ±3.21%
url/url-resolve.js n=100000 path='up' href='file'                      ***      4.69 %       ±1.53% ±2.04% ±2.66%
url/url-resolve.js n=100000 path='up' href='idn'                       ***      5.06 %       ±1.39% ±1.85% ±2.41%
url/url-resolve.js n=100000 path='up' href='javascript'                ***      4.17 %       ±2.25% ±2.99% ±3.89%
url/url-resolve.js n=100000 path='up' href='long'                      ***      2.84 %       ±1.02% ±1.37% ±1.79%
url/url-resolve.js n=100000 path='up' href='noscheme'                  ***      4.83 %       ±1.36% ±1.81% ±2.35%
url/url-resolve.js n=100000 path='up' href='percent'                   ***      3.51 %       ±1.49% ±1.98% ±2.59%
url/url-resolve.js n=100000 path='up' href='short'                     ***      3.44 %       ±1.74% ±2.32% ±3.01%
url/url-resolve.js n=100000 path='up' href='ws'                        ***      4.40 %       ±1.42% ±1.89% ±2.47%
url/url-resolve.js n=100000 path='withscheme' href='auth'              ***      5.94 %       ±1.64% ±2.19% ±2.85%
url/url-resolve.js n=100000 path='withscheme' href='dot'               ***      7.80 %       ±1.36% ±1.81% ±2.36%
url/url-resolve.js n=100000 path='withscheme' href='file'              ***      6.84 %       ±1.65% ±2.19% ±2.85%
url/url-resolve.js n=100000 path='withscheme' href='idn'               ***      4.69 %       ±1.47% ±1.96% ±2.55%
url/url-resolve.js n=100000 path='withscheme' href='javascript'        ***      6.29 %       ±1.46% ±1.94% ±2.53%
url/url-resolve.js n=100000 path='withscheme' href='long'               **      1.86 %       ±1.24% ±1.66% ±2.17%
url/url-resolve.js n=100000 path='withscheme' href='noscheme'          ***      7.49 %       ±1.66% ±2.21% ±2.89%
url/url-resolve.js n=100000 path='withscheme' href='percent'           ***      5.68 %       ±1.64% ±2.19% ±2.85%
url/url-resolve.js n=100000 path='withscheme' href='short'             ***      5.01 %       ±2.05% ±2.73% ±3.55%
url/url-resolve.js n=100000 path='withscheme' href='ws'                ***      4.33 %       ±2.14% ±2.85% ±3.72%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 50 comparisons, you can thus expect the following amount of false-positive results:
  2.50 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.50 false positives, when considering a   1% risk acceptance (**, ***),
  0.05 false positives, when considering a 0.1% risk acceptance (***)

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/url

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. url Issues and PRs related to the legacy built-in url module. labels Jul 29, 2024
Copy link

codecov bot commented Jul 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.07%. Comparing base (01cf9bc) to head (f411305).
Report is 18 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54092      +/-   ##
==========================================
- Coverage   87.07%   87.07%   -0.01%     
==========================================
  Files         643      643              
  Lines      181583   181583              
  Branches    34886    34886              
==========================================
- Hits       158114   158112       -2     
  Misses      16751    16751              
- Partials     6718     6720       +2     
Files Coverage Δ
lib/url.js 97.88% <100.00%> (-0.02%) ⬇️

... and 31 files with indirect coverage changes

@daeyeon
Copy link
Member

daeyeon commented Jul 29, 2024

Benchmark CI: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/1586/

Results
                                                                confidence improvement accuracy (*)   (**)  (***)
url/url-resolve.js n=100000 path='down' href='auth'                    ***      5.42 %       ±0.24% ±0.32% ±0.41%
url/url-resolve.js n=100000 path='down' href='dot'                     ***      5.28 %       ±0.26% ±0.35% ±0.45%
url/url-resolve.js n=100000 path='down' href='file'                    ***      6.21 %       ±0.33% ±0.44% ±0.57%
url/url-resolve.js n=100000 path='down' href='idn'                     ***      5.85 %       ±0.27% ±0.36% ±0.47%
url/url-resolve.js n=100000 path='down' href='javascript'              ***      7.76 %       ±0.17% ±0.23% ±0.30%
url/url-resolve.js n=100000 path='down' href='long'                    ***      3.22 %       ±0.34% ±0.45% ±0.59%
url/url-resolve.js n=100000 path='down' href='noscheme'                ***      6.51 %       ±0.24% ±0.32% ±0.41%
url/url-resolve.js n=100000 path='down' href='percent'                 ***      6.50 %       ±0.33% ±0.44% ±0.58%
url/url-resolve.js n=100000 path='down' href='short'                   ***      4.67 %       ±0.26% ±0.34% ±0.44%
url/url-resolve.js n=100000 path='down' href='ws'                      ***      4.98 %       ±0.19% ±0.25% ±0.33%
url/url-resolve.js n=100000 path='foo/bar' href='auth'                 ***      5.40 %       ±0.28% ±0.37% ±0.48%
url/url-resolve.js n=100000 path='foo/bar' href='dot'                  ***      7.26 %       ±0.25% ±0.33% ±0.43%
url/url-resolve.js n=100000 path='foo/bar' href='file'                 ***      7.06 %       ±0.20% ±0.26% ±0.34%
url/url-resolve.js n=100000 path='foo/bar' href='idn'                  ***      6.02 %       ±0.21% ±0.29% ±0.37%
url/url-resolve.js n=100000 path='foo/bar' href='javascript'           ***      6.48 %       ±0.37% ±0.50% ±0.66%
url/url-resolve.js n=100000 path='foo/bar' href='long'                 ***      3.02 %       ±0.25% ±0.33% ±0.43%
url/url-resolve.js n=100000 path='foo/bar' href='noscheme'             ***      7.47 %       ±0.24% ±0.32% ±0.42%
url/url-resolve.js n=100000 path='foo/bar' href='percent'              ***      7.50 %       ±0.28% ±0.38% ±0.49%
url/url-resolve.js n=100000 path='foo/bar' href='short'                ***      7.56 %       ±0.38% ±0.51% ±0.67%
url/url-resolve.js n=100000 path='foo/bar' href='ws'                   ***      5.79 %       ±0.20% ±0.27% ±0.35%
url/url-resolve.js n=100000 path='sibling' href='auth'                 ***      5.48 %       ±0.21% ±0.29% ±0.37%
url/url-resolve.js n=100000 path='sibling' href='dot'                  ***      5.49 %       ±0.35% ±0.46% ±0.60%
url/url-resolve.js n=100000 path='sibling' href='file'                 ***      5.74 %       ±0.24% ±0.32% ±0.41%
url/url-resolve.js n=100000 path='sibling' href='idn'                  ***      5.24 %       ±0.19% ±0.25% ±0.33%
url/url-resolve.js n=100000 path='sibling' href='javascript'           ***      5.93 %       ±0.26% ±0.35% ±0.46%
url/url-resolve.js n=100000 path='sibling' href='long'                 ***      3.05 %       ±0.27% ±0.36% ±0.47%
url/url-resolve.js n=100000 path='sibling' href='noscheme'             ***      6.30 %       ±0.20% ±0.27% ±0.35%
url/url-resolve.js n=100000 path='sibling' href='percent'              ***      6.38 %       ±0.25% ±0.34% ±0.44%
url/url-resolve.js n=100000 path='sibling' href='short'                ***      4.98 %       ±0.20% ±0.26% ±0.34%
url/url-resolve.js n=100000 path='sibling' href='ws'                   ***      4.12 %       ±0.30% ±0.40% ±0.52%
url/url-resolve.js n=100000 path='up' href='auth'                      ***      4.26 %       ±0.36% ±0.48% ±0.63%
url/url-resolve.js n=100000 path='up' href='dot'                       ***      4.88 %       ±0.35% ±0.47% ±0.61%
url/url-resolve.js n=100000 path='up' href='file'                      ***      6.06 %       ±0.22% ±0.29% ±0.38%
url/url-resolve.js n=100000 path='up' href='idn'                       ***      6.37 %       ±0.24% ±0.32% ±0.42%
url/url-resolve.js n=100000 path='up' href='javascript'                ***      5.91 %       ±0.43% ±0.57% ±0.75%
url/url-resolve.js n=100000 path='up' href='long'                      ***      4.04 %       ±0.37% ±0.49% ±0.64%
url/url-resolve.js n=100000 path='up' href='noscheme'                  ***      6.36 %       ±0.23% ±0.30% ±0.40%
url/url-resolve.js n=100000 path='up' href='percent'                   ***      5.22 %       ±0.27% ±0.36% ±0.46%
url/url-resolve.js n=100000 path='up' href='short'                     ***      6.10 %       ±0.25% ±0.34% ±0.44%
url/url-resolve.js n=100000 path='up' href='ws'                        ***      5.21 %       ±0.32% ±0.43% ±0.56%
url/url-resolve.js n=100000 path='withscheme' href='auth'              ***      6.80 %       ±0.33% ±0.44% ±0.58%
url/url-resolve.js n=100000 path='withscheme' href='dot'               ***      7.05 %       ±0.47% ±0.62% ±0.81%
url/url-resolve.js n=100000 path='withscheme' href='file'              ***      7.86 %       ±0.48% ±0.64% ±0.83%
url/url-resolve.js n=100000 path='withscheme' href='idn'               ***      6.00 %       ±0.28% ±0.38% ±0.49%
url/url-resolve.js n=100000 path='withscheme' href='javascript'        ***      8.50 %       ±0.45% ±0.60% ±0.78%
url/url-resolve.js n=100000 path='withscheme' href='long'              ***      3.09 %       ±0.29% ±0.39% ±0.51%
url/url-resolve.js n=100000 path='withscheme' href='noscheme'          ***      6.98 %       ±0.30% ±0.40% ±0.52%
url/url-resolve.js n=100000 path='withscheme' href='percent'           ***      8.57 %       ±0.77% ±1.04% ±1.37%
url/url-resolve.js n=100000 path='withscheme' href='short'             ***      7.86 %       ±0.43% ±0.57% ±0.74%
url/url-resolve.js n=100000 path='withscheme' href='ws'                ***      5.92 %       ±0.42% ±0.56% ±0.73%

@daeyeon daeyeon added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 29, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 29, 2024
@nodejs-github-bot
Copy link
Collaborator

@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 30, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 30, 2024
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 31, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/54092
✔  Done loading data for nodejs/node/pull/54092
----------------------------------- PR info ------------------------------------
Title      url: improve resolveObject with ObjectAssign (#54092)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     EarlyRiser42:resolveObject -> nodejs:main
Labels     url, author ready, needs-ci
Commits    1
 - url: improve resolveObject with ObjectAssign
Committers 1
 - EarlyRiser42 <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54092
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54092
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - url: improve resolveObject with ObjectAssign
   ℹ  This PR was created on Mon, 29 Jul 2024 05:08:45 GMT
   ✔  Approvals: 3
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/54092#pullrequestreview-2205488049
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/54092#pullrequestreview-2205511559
   ✔  - Daeyeon Jeong (@daeyeon): https://github.com/nodejs/node/pull/54092#pullrequestreview-2206281172
   ✔  Last GitHub CI successful
   ℹ  Last Benchmark CI on 2024-07-29T23:18:05Z: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/1586/
   ℹ  Last Full PR CI on 2024-07-31T01:51:04Z: https://ci.nodejs.org/job/node-test-pull-request/60758/
- Querying data for job/node-test-pull-request/60758/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/10174423900

@daeyeon daeyeon removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jul 31, 2024
@daeyeon daeyeon added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 31, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 31, 2024
@nodejs-github-bot nodejs-github-bot merged commit b4fd1fd into nodejs:main Jul 31, 2024
59 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in b4fd1fd

@EarlyRiser42 EarlyRiser42 deleted the resolveObject branch July 31, 2024 08:44
targos pushed a commit that referenced this pull request Aug 14, 2024
PR-URL: #54092
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants