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 — perf_hooks: refactor internals by jasnell · Pull Request #17822 · nodejs/node

Conversation

@jasnell

@jasnell jasnell commented Dec 22, 2017

Copy link
Copy Markdown
Member

Refactor and simplify the perf_hooks native internals.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

perf_hooks

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jasnell
perf_hooks: refactor internals
0e104a3 
Refactor and simplify the perf_hooks native internals.
@jasnell jasnell added the perf_hooks Issues and PRs related to the perf_hooks module and performance measurement APIs. label Dec 22, 2017
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 22, 2017
@jasnell

jasnell commented Dec 22, 2017

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax approved these changes Dec 22, 2017
View reviewed changes
Comment thread src/node_perf.cc Outdated
v8::GCType performance_last_gc_type_ = v8::GCType::kGCTypeAll;

// Initialize the performance entry object properties
inline void InitObject(PerformanceEntry* entry, Local<Object> obj) {

@addaleax addaleax Dec 22, 2017

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.

const PerformanceEntry& entry? Makes it a bit more obvious which parameter is the source/target

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/node_perf.h Outdated
virtual ~PerformanceEntry() { }

~PerformanceEntry() {}
virtual Local<Object> ToObject();

@addaleax addaleax Dec 22, 2017

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.

This could/should be const as well I think?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jasnell
[Squash] const the things
76f26e7
@addaleax

addaleax commented Dec 27, 2017

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-commit/15081/
jasnell 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.
@addaleax addaleax added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Dec 27, 2017
addaleax pushed a commit that referenced this pull request Dec 27, 2017
@jasnell @addaleax
perf_hooks: refactor internals
9e5ccf0 
Refactor and simplify the perf_hooks native internals.

PR-URL: #17822
Reviewed-By: Anna Henningsen <[email protected]>
@addaleax

addaleax commented Dec 27, 2017

Copy link
Copy Markdown
Member

Landed in 9e5ccf0
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax closed this Dec 27, 2017
@addaleax addaleax removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Dec 27, 2017
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@jasnell @MylesBorins
perf_hooks: refactor internals
07289c8 
Refactor and simplify the perf_hooks native internals.

PR-URL: #17822
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@jasnell @MylesBorins
perf_hooks: refactor internals
6fa2a9f 
Refactor and simplify the perf_hooks native internals.

PR-URL: #17822
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@jasnell @MylesBorins
perf_hooks: refactor internals
a329cf6 
Refactor and simplify the perf_hooks native internals.

PR-URL: #17822
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
v9.4.0 proposal #18069
Merged
@gibfahn gibfahn mentioned this pull request Jan 24, 2018
Add dont-land-on-v6.x label for perf_hooks PRs nodejs/github-bot#165
Closed
@gibfahn gibfahn added dont-land-on-v6.x labels Jan 24, 2018
@gibfahn

gibfahn commented Jan 24, 2018

Copy link
Copy Markdown
Member

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
@jasnell @kjin
perf_hooks: refactor internals
d5ddbd9 
Refactor and simplify the perf_hooks native internals.

PR-URL: nodejs#17822
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
@jasnell @MylesBorins
perf_hooks: refactor internals
54442ef 
Refactor and simplify the perf_hooks native internals.

Backport-PR-URL: #20456
PR-URL: #17822
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
v8.11.2 proposal #20478
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. perf_hooks Issues and PRs related to the perf_hooks module and performance measurement APIs.

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.

4 participants

@jasnell @addaleax @gibfahn @nodejs-github-bot
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.