Skip to content

lib: make navigator properties lazy - #53649

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
jasnell:make-navigator-properties-lazy
Jul 4, 2024
Merged

lib: make navigator properties lazy#53649
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
jasnell:make-navigator-properties-lazy

Conversation

@jasnell

Copy link
Copy Markdown
Member

Noticed in some benchmarking/profiling that the Navigator object constructor was rather expensive and slow due to initialization of properties during construction. It makes more sense for these to be lazily initialized on first access.

Flamegraphs... of `node -pe "navigator"

Before:
image

After:
image

@jasnell
jasnell requested review from anonrig and mcollinaJune 30, 2024 07:48
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/web-standards

@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Jun 30, 2024
@jasnelljasnell added the performance Issues and PRs related to the performance of Node.js. label Jun 30, 2024
@jasnell
jasnellforce-pushed the make-navigator-properties-lazy branch from 5a3b5a3 to 3d2e10fCompareJune 30, 2024 07:53
Noticed in some benchmarking/profiling that the Navigator object
constructor was rather expensive and slow due to initialization
of properties during construction. It makes more sense for these
to be lazily initialized on first access.
@jasnell
jasnellforce-pushed the make-navigator-properties-lazy branch from 3d2e10f to 875952bCompareJune 30, 2024 07:56
Comment threadlib/internal/navigator.js

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@anonriganonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jun 30, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 3, 2024
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@richardlaurichardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 4, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 4, 2024
@nodejs-github-bot
nodejs-github-bot merged commit 8040994 into nodejs:mainJul 4, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 8040994

aduh95 pushed a commit that referenced this pull request Jul 12, 2024
Noticed in some benchmarking/profiling that the Navigator object
constructor was rather expensive and slow due to initialization
of properties during construction. It makes more sense for these
to be lazily initialized on first access.
PR-URL: #53649
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@aduh95aduh95 mentioned this pull request Jul 12, 2024
ljharb added a commit to ljharb/node that referenced this pull request Jul 15, 2024
nodejs-github-bot pushed a commit that referenced this pull request Jul 15, 2024
Preserves #53649.
PR-URL: #53765
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Jul 16, 2024
Noticed in some benchmarking/profiling that the Navigator object
constructor was rather expensive and slow due to initialization
of properties during construction. It makes more sense for these
to be lazily initialized on first access.
PR-URL: #53649
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
aduh95 pushed a commit that referenced this pull request Jul 16, 2024
Preserves #53649.
PR-URL: #53765
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
ehsankhfr pushed a commit to ehsankhfr/node that referenced this pull request Jul 18, 2024
Preserves nodejs#53649.
PR-URL: nodejs#53765
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.needs-ciPRs that need a full CI run.performanceIssues and PRs related to the performance of Node.js.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@jasnell@nodejs-github-bot@mcollina@panva@benjamingr@anonrig@tniessen@MoLow@targos@richardlau