lttng: updated flags for gc tracing #3388
Conversation
mscdex
added
the
post-mortem
|
cc @nodejs/lts |
|
@thekemkid do you know how long this has been broken? it'd be nice if we could get some lttng folks using releases more often so this stuff was kept in shape. |
|
@rvagg it looks like it was working in the v4.1.2 proposal branch. I'm planning on doing some more work on lttng/adding more lttng tracepoints so I'll keep maintaining it. :) |
| flagsStr = "kGCCallbackFlagConstructRetainedObjectInfos"; | ||
| } else if (flags == v8::GCCallbackFlags::kGCCallbackFlagForced) { | ||
| flagsStr = "kGCCallbackFlagForced"; | ||
| } else if (flags == v8::GCCallbackFlags::kGCCallbackFlagSynchronousPhantomCallbackProcessing) { |
There was a problem hiding this comment.
Long lines, please keep them <= 80 columns.
There was a problem hiding this comment.
Would it be okay if I used a switch here instead?
Heres what I'm thinking:
switch (flags) {
case
v8::GCCallbackFlags::kNoGCCallbackFlags:
flagsStr = "kNoGCCallbackFlags";
break;
case
v8::GCCallbackFlags::kGCCallbackFlagConstructRetainedObjectInfos;
flagsStr = "kGCCallbackFlagConstructRetainedObjectInfos";
break;
case
v8::GCCallbackFlags::kGCCallbackFlagForced:
flagsStr = "kGCCallbackFlagForced";
break;
case
v8::GCCallbackFlags::kGCCallbackFlagSynchronousPhantomCallbackProcessing:
flagsStr = "kGCCallbackFlagSynchronousPhantomCallbackProcessing";
break;
default:
flagsStr = "Unrecognised GCCallbackFlag";
break;
}There was a problem hiding this comment.
That looks a little awkward. You could add a using F = v8::GCCallbackFlags a few lines up so you can shorten the names but I'd probably use an x-macro to generate the clauses. That lets you drop the duplication of the enum and the string as well. You can find an example of an x-macro in src/node_v8.cc, grep for HEAP_STATISTICS_PROPERTIES.
There was a problem hiding this comment.
Done! I also shortened the GCType if statement with an x-macro too.
|
The breakage is because of the recent upgrade to V8 4.6 in master. |
|
Okay, I think this is ready to be reviewed again :) Let me know if I need to squash the commits. |
|
could I get another review? @nodejs/lts |
PR-URL: #3388 Reviewed-By: Ben Noordhuis <[email protected]>
|
Landed in 9adc6a6, thanks. Can you make sure next time that |
|
Should we add an LTS tag here? |
No need. :) |
|
@bnoordhuis will do, thanks :) |
PR-URL: #3388 Reviewed-By: Ben Noordhuis <[email protected]>
This fixes some build errors I was getting while trying to build --with-lttng. :)