FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

add --track-heap-objects by bmeck · Pull Request #2135 · nodejs/node · GitHub

/ node Public

add --track-heap-objects - #2135

Closed
bmeck wants to merge 3 commits into
nodejs:masterfrom
bmeck:track-heap-objects
ConversationCommits (3)ChecksFiles changed
Closed

add --track-heap-objects#2135
bmeck wants to merge 3 commits into
nodejs:masterfrom
bmeck:track-heap-objects

Conversation

bmeck commented Jul 8, 2015

Copy link
Copy Markdown
Member

Right now we do not have a way to start iojs with heap object tracking.

Calling v8::Profiler::StartTrackingHeapObjects after node has started up will not include the reference info for objects in node core. This can be seen by running node-heapdump with the flag turned on. This information is useful for debugging tools and would be visible in Chrome DevTools in the same way that "Record heap allocation stack traces" functions from the DevTools settings pane.

Fishrock123 added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. labels Jul 8, 2015

ofrobots commented Jul 8, 2015

Copy link
Copy Markdown
Contributor

Does doc/iojs.1 also need to be modified?

thefourtheye commented Jul 8, 2015

Copy link
Copy Markdown
Contributor

I am not able to understand why we need the whitespace changes.

bmeck commented Jul 8, 2015

Copy link
Copy Markdown
Member Author

@thefourtheye it was to keep 2 spaces between the switch and the description and keeping all of the descriptions aligned

bmeck commented Jul 8, 2015

Copy link
Copy Markdown
Member Author

@ofrobots i can but I saw that --trace-sync-io was not in there either

thefourtheye commented Jul 8, 2015

Copy link
Copy Markdown
Contributor

@bmeck Ah, I see what you are saying now. " --track-heap-objects track heap object allocations for heap snapshots\n", correct? Not a v8 expert, but the changes look straight-forward.

bmeck commented Jul 8, 2015

Copy link
Copy Markdown
Member Author

@thefourtheye correct

thefourtheye reviewed Jul 8, 2015
View reviewed changes
Comment thread src/node.cc Outdated

thefourtheye Jul 8, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Shouldn't we explicitly set this to False, in the else part?

bmeck Jul 8, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

none of the other switches work that way

thefourtheye Jul 8, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

👍

bnoordhuis Jul 8, 2015

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Does it even need to be exposed to JS land?

(I guess the same question applies to .traceSyncIO, /cc @trevnorris.)

trevnorris Jul 9, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yup. You're right. I can't think of a useful case to expose either of them to JS.

bnoordhuis Jul 9, 2015

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good to know. @bmeck Can you remove the .trackHeapObjects property (and .traceSyncIO in a separate commit if you want)?

bmeck commented Jul 9, 2015

Copy link
Copy Markdown
Member Author

@bnoordhuis removed trackHeapObjects from process, separate PR for traceSyncIO #2143

bnoordhuis commented Jul 9, 2015

Copy link
Copy Markdown
Member

LGTM

trevnorris commented Jul 9, 2015

Copy link
Copy Markdown
Contributor

@bmeck Not adding that extra documentation was a failure on my part. If all other options are listed then this and the other should as well.

EDIT: comment was meant for @ofrobots

trevnorris commented Jul 9, 2015

Copy link
Copy Markdown
Contributor

LGTM

bmeck force-pushed the track-heap-objects branch from 3fb4779 to eaa89b7 Compare July 9, 2015 16:24

thefourtheye commented Jul 9, 2015

Copy link
Copy Markdown
Contributor

Do we need a test for this?

bmeck commented Jul 9, 2015

Copy link
Copy Markdown
Member Author

@thefourtheye node itself does not include a way to dump heap snapshots, we could write one up but it would be akin to including node-heapdump in core.

bmeck added 3 commits July 10, 2015 16:06
add --track-heap-objects
9748d60 
- This makes v8 add .trace_function_info to the serialized form of
  snapshots from v8::HeapSnapshot::Serialize
- .trace_funciton_info combined with .trace_node in snapshots tells the
  JS location that allocated a specific object
style changes, move tracking to earlier start
8e2490c
-h space between heapsnapshot
bd77a59
bmeck force-pushed the track-heap-objects branch from eaa89b7 to bd77a59 Compare July 10, 2015 21:24
trevnorris pushed a commit that referenced this pull request Jul 10, 2015
src: add --track-heap-objects
cf14a24 
- This makes v8 add .trace_function_info to the serialized form of
  snapshots from v8::HeapSnapshot::Serialize
- .trace_funciton_info combined with .trace_node in snapshots tells the
  JS location that allocated a specific object

PR-URL: #2135
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

trevnorris commented Jul 10, 2015

Copy link
Copy Markdown
Contributor

Squashed and landed in cf14a24.

trevnorris closed this Jul 10, 2015
Fishrock123 added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 12, 2015

Fishrock123 commented Jul 12, 2015

Copy link
Copy Markdown
Contributor

Tagged as semver-minor, 2.4.0 ho! :)

Fishrock123 mentioned this pull request Jul 16, 2015
Merged
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jul 17, 2015
2015-07-17 io.js v2.4.0 Release
a179bcb 
Notable changes

* src: Added a new `--track-heap-objects` flag to track heap object
allocations for heap snapshots (Bradley Meck)
nodejs#2135.
* readline: Fixed a freeze that affected the repl if the keypress event
handler threw (Alex Kocharin) nodejs#2107.
* npm: Upgraded to v2.13.0, release notes can be found in
https://github.com/npm/npm/releases/tag/v2.13.0 (Forrest L Norvell)
nodejs#2152.

PR-URL: nodejs#2189
snyk-bot mentioned this pull request Apr 1, 2020
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL