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

feat!: use inspector for time profiles #227

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

aabmass
Copy link
Member

@aabmass aabmass commented Oct 21, 2022

For time profiling, replaces the native C++ addon with calls to Node's inspector module. This uses the same V8::CpuProfiler under the hood but communicates over the chrome devtools protocol. More context: #170 (comment)

Once the change is also made for heap profilers, this will obsolete the C++ code and we will no longer need any prebuilt binaries.

Breaking changes 📢

Testing

I wrote a test script to generate some profiles and check for any differences. The flame graphs look the same:
image

Main differences I can see in the peek view of the profiles:
image

@aabmass aabmass force-pushed the use-inspector-cpu branch 2 times, most recently from f919d24 to a0ee1a7 Compare October 21, 2022 03:48
@aabmass aabmass changed the title Use inspector for cpu profiles feat!: use inspector for time profiles Oct 21, 2022
ts/src/index.ts Outdated Show resolved Hide resolved
ts/src/time-profiler-inspector.ts Show resolved Hide resolved
ts/src/time-profiler-inspector.ts Outdated Show resolved Hide resolved
}

export function start(
export async function start(
Copy link
Member Author

Choose a reason for hiding this comment

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

This now has to be async which is another breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a chance (depending on underlying implementation) this will result in a fix for googleapis/cloud-profiler-nodejs#683 (event loop blocked at start of profile collection).

This would be a good change if it does that (though needing to update agent code will be a bummer)

Copy link
Member Author

Choose a reason for hiding this comment

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

Great! I will assume this fixes that issue and close it once this change gets pull into that repo.

@nolanmar511
Copy link
Contributor

On the off-by-one line numbers & differing file names, the existing system tests checking that mapping of javascript to typescript (uses busybench.ts; here) might be helpful.

As long as the mapping of javascript => typescript works, any slight changes in file names and line numbers seem fine to me.

ts/src/time-profiler-inspector.ts Outdated Show resolved Hide resolved
ts/src/time-profiler-inspector.ts Show resolved Hide resolved
ts/src/time-profiler-inspector.ts Show resolved Hide resolved
ts/src/time-profiler-inspector.ts Show resolved Hide resolved
ts/src/time-profiler-inspector.ts Show resolved Hide resolved
@punya
Copy link
Collaborator

punya commented Oct 21, 2022

Node 12 is EOL so I think we should be okay with dropping that from CI.

The inspector approach uses
[`CpuProfilingMode::kLeafNodeLineNumbers`](https://source.chromium.org/chromium/chromium/src/+/main:v8/include/v8-profiler.h;l=244;drc=27e39fa1dd71076618c358639ed8a327bc3873c4)
under the hood which changes the line numbering scheme. I updated the
tests to expect the new line number.
@aabmass aabmass marked this pull request as ready for review November 2, 2022 04:18
@weyert
Copy link

weyert commented Nov 18, 2022

What's the status of this PR? Really interested in trying out Cloud Profiler with Node LTS :D

@aabmass
Copy link
Member Author

aabmass commented Nov 18, 2022

What's the status of this PR? Really interested in trying out Cloud Profiler with Node LTS :D

@weyert We want to verify that HeapProfiler through inspector is working correctly too. Then we can remove all the native code and make a release

@weyert
Copy link

weyert commented Nov 18, 2022

@weyert We want to verify that HeapProfiler through inspector is working correctly too. Then we can remove all the native code and make a release

Thanks for the update, have to admit not fully following what you are saying but I wish you good luck finding out :D

@psx95
Copy link
Contributor

psx95 commented Nov 18, 2022

@weyert We want to verify that HeapProfiler through inspector is working correctly too. Then we can remove all the native code and make a release

Thanks for the update, have to admit not fully following what you are saying but I wish you good luck finding out :D

So, the in-built node profiler also has heap profiler functionality. We need to verify if that can be used to replace heap profiling here. If it works, we can make a new release which uses node's in-built profilers for profiling both - CPU & Memory instead of using native code which we currently do.

@weyert
Copy link

weyert commented Nov 30, 2022

Any news on the support for Node 16/18?

@aabmass
Copy link
Member Author

aabmass commented Dec 1, 2022

@weyert the current released version should already work with those versions of node, but there are no precompiled binaries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants