-
Notifications
You must be signed in to change notification settings - Fork 109
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
Fixed support for Node 8.12.0. #172
Conversation
11c8943
to
bd949d4
Compare
lib/v8-log-to-ticks.js
Outdated
@@ -22,8 +43,11 @@ function v8LogToTicks (isolateLogPath, node) { | |||
// it's an error in the V8 internal tick profiler) | |||
// ignore them. | |||
const ignore = /unknown code state:/ | |||
// pump(sp.stderr, process.stderr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obsolete comment? :)
lib/v8-log-to-ticks.js
Outdated
|
||
module.exports = process | ||
|
||
function process (isolateLogPath, node) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a different name? Having the same name as global seems not ideal.
lib/v8-log-to-ticks.js
Outdated
module.exports = process | ||
|
||
function process (isolateLogPath, node) { | ||
return filterLinesThatAreTooLong(isolateLogPath).then((filtered) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to only execute this when it's necessary (by checking the Node.js version).
lib/v8-log-to-ticks.js
Outdated
return readFile(isolateLogPath, 'utf8').then((data) => { | ||
data = data.split('\n').filter((s) => s.length < 1024).join('\n') | ||
return writeFile(dest, data) | ||
}).then(() => dest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: use async / await.
bd949d4
to
7d895ad
Compare
PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (even though I would still argue it that only doing it in 8 and not in 10 where it's already fixed would be best).
Those lines cause the processor to fail, and we silence that error. I think this is just avoid future problems. |
@mcollina this doesn't seem to always fix the problem -- I'm still getting the error with lines > 1024 characters removed. Maybe something else besides long lines causes My workaround was to evaluate https://github.com/nodejs/node/blob/2ae98ce7cb95dee79e839afb2ee4c7d77d7670cd/lib/internal/v8_prof_processor.js in a new JS file, and the errors that were logged were all |
Can you upload an example to reproduce? Thanks!
Il giorno mer 17 ott 2018 alle 04:00 Zach Bjornson <[email protected]>
ha scritto:
… @mcollina <https://github.com/mcollina> this doesn't seem to always fix
the problem -- I'm still getting the error with lines > 1024 characters
removed. Maybe something else besides long lines causes printErr to be
invoked?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#172 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADL48cCIzKlM6difeCY9CTbgXgEhtQ7ks5ulo8qgaJpZM4WwD5Q>
.
|
Here's one: isolate-0x3aad0c0-v8.log |
No description provided.