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

src: refactor V8ProfilerConnection to be more reusable #27475

Closed
wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Apr 29, 2019

src: refactor profile initialization

  • Process and store --cpu-prof-dir and --cpu-prof-name during
    Environment creation
  • Start profilers in on profiler::StartProfilers()

src: refactor V8ProfilerConnection to be more reusable

Now the subclasses only need to override methods to

  • Get the profile node from the profiler response message object
  • Return the directory and file path

And the V8ProfilerConnection() would handle the rest of profile
serialization and message parsing. This makes it easier to
add other types of profiles in the future.

Refs: #27421

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

- Process and store --cpu-prof-dir and --cpu-prof-name during
  Environment creation
- Start profilers in on `profiler::StartProfilers()`
Now the subclasses only need to override methods to

- Get the profile node from the profiler response message object
- Return the directory and file path

And the V8ProfilerConnection() would handle the rest of profile
serialization and message parsing. This makes it easier to
add other types of profiles in the future.
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Apr 29, 2019
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Looks good!

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

The first commit log has an extraneous "on" in "Start profilers in on profiler::StartProfilers()".

src/inspector_profiler.cc Show resolved Hide resolved
src/inspector_profiler.cc Outdated Show resolved Hide resolved
src/inspector_profiler.cc Outdated Show resolved Hide resolved
src/inspector_profiler.cc Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

joyeecheung commented May 1, 2019

Fixed punctuation of comments and left some TODOs for @bnoordhuis 's reviews.

The first commit log has an extraneous "on" in "Start profilers in on profiler::StartProfilers()".

Oops, I meant one instead of on. Will fix it when landing the commit.

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

joyeecheung commented May 2, 2019

Landed in 6f02f15...58ba952 with the comment about mkdirp removed. Thanks for the explanations ! @bnoordhuis

joyeecheung added a commit that referenced this pull request May 2, 2019
- Process and store --cpu-prof-dir and --cpu-prof-name during
  Environment creation
- Start profilers in one `profiler::StartProfilers()`

PR-URL: #27475
Refs: #27421
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
joyeecheung added a commit that referenced this pull request May 2, 2019
Now the subclasses only need to override methods to

- Get the profile node from the profiler response message object
- Return the directory and file path

And the V8ProfilerConnection() would handle the rest of profile
serialization and message parsing. This makes it easier to
add other types of profiles in the future.

PR-URL: #27475
Refs: #27421
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@joyeecheung joyeecheung closed this May 2, 2019
targos pushed a commit that referenced this pull request May 4, 2019
- Process and store --cpu-prof-dir and --cpu-prof-name during
  Environment creation
- Start profilers in one `profiler::StartProfilers()`

PR-URL: #27475
Refs: #27421
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request May 4, 2019
Now the subclasses only need to override methods to

- Get the profile node from the profiler response message object
- Return the directory and file path

And the V8ProfilerConnection() would handle the rest of profile
serialization and message parsing. This makes it easier to
add other types of profiles in the future.

PR-URL: #27475
Refs: #27421
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@targos targos mentioned this pull request May 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants