node icon indicating copy to clipboard operation
node copied to clipboard

lib: reduce url getters on `makeRequireFunction`

Open anonrig opened this issue 2 years ago • 12 comments

Reduce calling getters of URL due to the lazy loading of URL.

Ref: https://github.com/nodejs/performance/issues/92

anonrig avatar Jun 18 '23 16:06 anonrig

Review requested:

  • [ ] @nodejs/loaders
  • [ ] @nodejs/modules

nodejs-github-bot avatar Jun 18 '23 16:06 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/52298/

nodejs-github-bot avatar Jun 20 '23 23:06 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/52300/

nodejs-github-bot avatar Jun 21 '23 03:06 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/52314/

nodejs-github-bot avatar Jun 21 '23 14:06 nodejs-github-bot

Thank you for this!

I gather from this and related PRs that URL objects in general are more costly than primitive strings? Should we make an effort to try to scrub the modules code of URL objects whenever possible, converting to strings instead?

@GeoffreyBooth URL object is not costly, but accessing their attributes except href is costly. For example, calling url.protocol multiple times, will create multiple different strings, rather than the same one. This is caused by the lazy getters of the class. I'm currently trying to reduce URL initializations and multiple getters of the same URL instance across the loaders.

anonrig avatar Jun 21 '23 16:06 anonrig

For example, calling url.protocol multiple times, will create multiple different strings, rather than the same one.

I would think it should be possible to “cache” protocol in whatever way that href is currently stored? So that calling protocol repeatedly is no more costly than calling href, assuming the latter is “inexpensive” (in the sense that it’s not worth trying to refactor to something even simpler).

GeoffreyBooth avatar Jun 21 '23 16:06 GeoffreyBooth

I would think it should be possible to “cache” protocol in whatever way that href is currently stored? So that calling protocol repeatedly is no more costly than calling href, assuming the latter is “inexpensive” (in the sense that it’s not worth trying to refactor to something even simpler).

This might be an (micro)-optimization worth investigating for, but adding a branch and accessing the cache parameter overhead might be greater than just simply reduce calling it. I simply don't have the data right now to back the claim if one approach is better than another.

anonrig avatar Jun 21 '23 16:06 anonrig

CI: https://ci.nodejs.org/job/node-test-pull-request/52326/

nodejs-github-bot avatar Jun 21 '23 17:06 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/52333/

nodejs-github-bot avatar Jun 21 '23 20:06 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/52346/

nodejs-github-bot avatar Jun 22 '23 15:06 nodejs-github-bot

Rebased and force-pushed.

anonrig avatar Jun 23 '23 17:06 anonrig

CI: https://ci.nodejs.org/job/node-test-pull-request/52400/

nodejs-github-bot avatar Jun 23 '23 17:06 nodejs-github-bot

Landed in 578ffe1edb3ee2acbe89b9db5555d966209584ba

nodejs-github-bot avatar Jun 24 '23 00:06 nodejs-github-bot