-
Notifications
You must be signed in to change notification settings - Fork 29
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
Have an e2e test against a node version that uses v8 very close to head #188
Comments
This is a good idea. Having feedback from potentially breaking changes in V8 as early as possible would be great. I am not sure whether the tests we have in the Node project are as extensive for the profiling/diagnostics use-cases, and this project might well be one of the most advanced users of these V8 APIs. In the node project we run If we want a build that is within O(hours) up to date, we can use the tarballs from the V8 integration build: https://uberchromegw.corp.google.com/i/client.v8.fyi/builders/V8%20Linux64%20-%20node.js%20integration. This build integrates each V8-roll (small number of commits) into a semi-recent Node.js snapshot. The built binaries are available for download as a tarball from that site too. This will provide semi-immediate feedback if a V8 commit ends up breaking us. |
O(days) is good but O(hours) is better so I'd go for that. |
Installing the v8-canary builds can be done in one line:
Installing the v8 integration build would be harder. There's some javascript code which does this, but it first retrieves retrieves a list of builds, then finds the latest successful build, then gets the build git revision and commit and then downloads the build. I don't think we'll need to get the revision and commit (those seem to be used to in reporting). |
@nolanmar511 Enabling v8-canary build is probably a good first step. Empirically, how fresh v8 is in these builds? |
When I run node-gyp rebuild with the v8-canary version of node, I get an error:
Any recommendations? |
@nolanmar511 Did you mean to tag the person your question was addressed to? |
I asked ofrobots in meeting. Since we'll need to identify what the commit being used is, it's probably necessary to use v8-lkgr. |
@ofrobots @nolanmar511 Based on discussion today looks like we'll use the LKGR build. Still need to figure out how get the matching V8 headers. |
I would like to re-open this as I think using the Node.js canary build may be simple enough. While verifying recent CPU profiler memory usage improvements, I used this command to install the canary build:
With that I indeed had issues installing the profiler module:
But using this trick makes things work:
And the headers in that location seems recent enough, at least they do include the recent line mode changes:
With that, adding testing against the canary build should be trivial? |
I tried using --nodedir where the profiler is installed (so Here's the complete output of the test run: |
@nolanmar511 Does it reproduce locally? Try to reproduce it locally, generate a core file and get the stack of the crash from the core file using gbd, something along these lines. That should give some clues about what is going on. |
Yep. This reproduces locally. This is the back trace.
|
Steps to reproduce segfault:
|
I reproduce it with debug Node.js, see the information below. @psmarshall @ofrobots Any thoughts on this? This does not look to me like a mismatch between headers and the binary code (and I am pretty sure the headers used the profiler bindings here were the right ones).
|
Oh, and I used |
FWIW, the code that faults is like this in canary-base:
but like this in master:
Maybe it will lead to some thoughts. |
Looks like this commit changed this code - v8/v8@f318f98. |
I added a comment on https://chromium-review.googlesource.com/c/v8/v8/+/1106380. |
I logged an upstream bug here: https://bugs.chromium.org/p/v8/issues/detail?id=8003 |
The original crash has been solved, but still having problems with v8 canary. Right now, when I try
|
@nolanmar511 I think the error happens because it tries to build the node from source and your machine doesn't have fully configured compiler. I would expect though that the canary location includes prebuilt node for linux-amd64 arch, and with nodejs/node#22757 it still doesn't seem to be the case. |
As CPU or heap profiler in v8 get any changes, we should continuously make sure those do not carry surprises for us (e.g. there is https://chromium-review.googlesource.com/c/v8/v8/+/1039825, which shouldn't break us but only green test gives confidence). To do that we need an automated e2e test run against a version of node that uses v8 very close to head. @ofrobots any recommendations on how to organize this? My naive hope would be that we could do something like "nvm install head" and the head gets v8 updated regularly (automatically?).
@nolanmar511 FYI
The text was updated successfully, but these errors were encountered: