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

add --track-heap-objects #2135

Closed
wants to merge 3 commits into from
Closed

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Jul 8, 2015

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 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
Copy link
Contributor

ofrobots commented Jul 8, 2015

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

@thefourtheye
Copy link
Contributor

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

@bmeck
Copy link
Member Author

bmeck commented Jul 8, 2015

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

@bmeck
Copy link
Member Author

bmeck commented Jul 8, 2015

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

@thefourtheye
Copy link
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
Copy link
Member Author

bmeck commented Jul 8, 2015

@thefourtheye correct

// --track-heap-objects
if (track_heap_objects) {
READONLY_PROPERTY(process, "trackHeapObjects", True(env->isolate()));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

none of the other switches work that way

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

Does it even need to be exposed to JS land?

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@bmeck
Copy link
Member Author

bmeck commented Jul 9, 2015

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

@bnoordhuis
Copy link
Member

LGTM

@trevnorris
Copy link
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
Copy link
Contributor

LGTM

@thefourtheye
Copy link
Contributor

Do we need a test for this?

@bmeck
Copy link
Member Author

bmeck commented Jul 9, 2015

@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.

- 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
trevnorris pushed a commit that referenced this pull request Jul 10, 2015
- 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
Copy link
Contributor

Squashed and landed in cf14a24.

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

Tagged as semver-minor, 2.4.0 ho! :)

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jul 17, 2015
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
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++. 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
Development

Successfully merging this pull request may close these issues.

None yet

6 participants