GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — lib: refactor to use more primordials in internal/histogram.js by RaisinTen · Pull Request #36455 · nodejs/node

Conversation

@RaisinTen

@RaisinTen RaisinTen commented Dec 9, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with eyes emoji
All reactions
  • 👀 1 reaction
aduh95
aduh95 approved these changes Dec 9, 2020
View reviewed changes
@targos

targos commented Dec 9, 2020

Copy link
Copy Markdown
Member

Why did you change private properties to symbol properties?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Dec 9, 2020

Copy link
Copy Markdown
Member

Why did you change private properties to symbol properties?

I presume because of the performance difference that still exists there (symbol are still faster). In this case, I would leave them as private fields.
RaisinTen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 requested changes Dec 10, 2020
View reviewed changes

@aduh95 aduh95 left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I agree with other comments here, using a symbol doesn't seem justified. An argument could be made for #handle/kHandle which is exported as a symbol anyway (although that would make possible for consumers to change the handle with another object, we'd have to make sure it won't introduce issues; I'd stick with the original code to not take any risk), but I suggest reverting changes to the #map private property.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RaisinTen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread lib/internal/histogram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/histogram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/histogram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/histogram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/histogram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/histogram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/histogram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed Dec 10, 2020
View reviewed changes

@aduh95 aduh95 left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

nit: it seems a good candidate for optional chaining

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/histogram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/histogram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/histogram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/histogram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/histogram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/histogram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/histogram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/histogram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/histogram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
RaisinTen
RaisinTen commented Dec 10, 2020
View reviewed changes
Comment thread lib/internal/histogram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen
RaisinTen marked this pull request as ready for review December 10, 2020 15:23
@RaisinTen
RaisinTen force-pushed the lib/refactor-to-use-more-primordials-internal/histogram.js branch from 8c6fb14 to 42a535b Compare December 10, 2020 15:23
aduh95
aduh95 approved these changes Dec 12, 2020
View reviewed changes

@aduh95 aduh95 left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM as long as kHandle is never exported to user-land (which seems to be the case).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ExE-Boss and RaisinTen reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Dec 12, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 12, 2020
@nodejs-github-bot

nodejs-github-bot commented Dec 12, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34913/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott approved these changes Dec 14, 2020
View reviewed changes

@Trott Trott left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM. Worth a benchmark of either quic or perf_hooks since those use this?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RaisinTen and ExE-Boss reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@aduh95 aduh95 added the needs-benchmark-ci PRs that need a benchmark CI run. label Dec 15, 2020
jasnell
jasnell approved these changes Dec 15, 2020
View reviewed changes
@Trott

Trott commented Dec 22, 2020

Copy link
Copy Markdown
Member

perf_hooks benchmark:https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/785/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Dec 22, 2020

Copy link
Copy Markdown
Member

Not sure if perf_hooks exercised the code here, but the results are good.

07:21:09  perf_hooks/bench-eventlooputil.js method='ELU_passed' n=1000000                -0.53 %       ±2.23% ±2.97% ±3.87%
07:21:09  perf_hooks/bench-eventlooputil.js method='ELU_simple' n=1000000                 0.26 %       ±3.09% ±4.12% ±5.37%
07:21:09  perf_hooks/bench-eventlooputil.js method='idleTime' n=1000000                   0.91 %       ±4.78% ±6.36% ±8.28%
RaisinTen reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen @aduh95 @Trott
lib: refactor to use more primordials in internal/histogram.js
1623aff 
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: nodejs#36455
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott Trott removed the needs-benchmark-ci PRs that need a benchmark CI run. label Dec 22, 2020
@Trott
Trott force-pushed the lib/refactor-to-use-more-primordials-internal/histogram.js branch from 42a535b to 1623aff Compare December 22, 2020 15:33
@Trott

Trott commented Dec 22, 2020

Copy link
Copy Markdown
Member

Landed in 1623aff
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott merged commit 1623aff into nodejs:master Dec 22, 2020
@RaisinTen
RaisinTen deleted the lib/refactor-to-use-more-primordials-internal/histogram.js branch December 22, 2020 15:47
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
@RaisinTen @aduh95 @danielleadams
lib: refactor to use more primordials in internal/histogram.js
63091f8 
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: #36455
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
v15.6.0 proposal #36889
Merged
targos pushed a commit that referenced this pull request May 16, 2021
@RaisinTen @aduh95 @targos
lib: refactor to use more primordials in internal/histogram.js
a988af4 
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: #36455
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Jun 6, 2021
v14.17.1 release proposal #38948
Merged
targos pushed a commit that referenced this pull request Jun 11, 2021
@RaisinTen @aduh95 @targos
lib: refactor to use more primordials in internal/histogram.js
e12dbc8 
Co-authored-by: Antoine du Hamel <[email protected]>
PR-URL: #36455
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@baby636 baby636 mentioned this pull request Nov 28, 2023
[Snyk] Fix for 37 vulnerabilities baby636/node#110
Open
@Renegade334 Renegade334 mentioned this pull request Jul 16, 2025
perf_hooks: do not expose SafeMap via Histogram wrapper #59094
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@Trott Trott Trott approved these changes
@aduh95 aduh95 aduh95 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@RaisinTen @targos @jasnell @nodejs-github-bot @Trott @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.