FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lttng: updated flags for gc tracing by GlenTiki · Pull Request #3388 · nodejs/node · GitHub

/ node Public

lttng: updated flags for gc tracing - #3388

Closed
GlenTiki wants to merge 2 commits into
nodejs:masterfrom
GlenTiki:master
ConversationCommits (2)ChecksFiles changed
Closed

lttng: updated flags for gc tracing#3388
GlenTiki wants to merge 2 commits into
nodejs:masterfrom
GlenTiki:master

Conversation

GlenTiki commented Oct 15, 2015

Copy link
Copy Markdown
Contributor

This fixes some build errors I was getting while trying to build --with-lttng. :)

mscdex added the post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. label Oct 15, 2015

Fishrock123 commented Oct 16, 2015

Copy link
Copy Markdown
Contributor

cc @nodejs/lts

rvagg commented Oct 16, 2015

Copy link
Copy Markdown
Member

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

GlenTiki commented Oct 16, 2015

Copy link
Copy Markdown
Contributor Author

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

bnoordhuis reviewed Oct 16, 2015
View reviewed changes
Comment thread src/node_lttng_provider.h Outdated

bnoordhuis Oct 16, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Long lines, please keep them <= 80 columns.

GlenTiki Oct 16, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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;
  }

bnoordhuis Oct 17, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

GlenTiki Oct 17, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done! I also shortened the GCType if statement with an x-macro too.

bnoordhuis commented Oct 16, 2015

Copy link
Copy Markdown
Member

The breakage is because of the recent upgrade to V8 4.6 in master.

GlenTiki commented Oct 17, 2015

Copy link
Copy Markdown
Contributor Author

Okay, I think this is ready to be reviewed again :) Let me know if I need to squash the commits.

GlenTiki commented Oct 20, 2015

Copy link
Copy Markdown
Contributor Author

could I get another review? @nodejs/lts

bnoordhuis pushed a commit that referenced this pull request Oct 20, 2015
lttng: update flags for gc tracing
9adc6a6 
PR-URL: #3388
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

bnoordhuis commented Oct 20, 2015

Copy link
Copy Markdown
Member

Landed in 9adc6a6, thanks. Can you make sure next time that make cpplint is happy?

bnoordhuis closed this Oct 20, 2015

MylesBorins commented Oct 20, 2015

Copy link
Copy Markdown
Contributor

Should we add an LTS tag here?

Fishrock123 commented Oct 20, 2015

Copy link
Copy Markdown
Contributor

The breakage is because of the recent upgrade to V8 4.6 in master.

Should we add an LTS tag here?

No need. :)

GlenTiki commented Oct 20, 2015

Copy link
Copy Markdown
Contributor Author

@bnoordhuis will do, thanks :)

rvagg pushed a commit that referenced this pull request Oct 21, 2015
lttng: update flags for gc tracing
d5ce534 
PR-URL: #3388
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
rvagg mentioned this pull request Oct 21, 2015
Merged
rvagg mentioned this pull request Oct 28, 2015
Closed
rvagg mentioned this pull request Dec 17, 2015
Closed

MylesBorins commented Dec 29, 2015

Copy link
Copy Markdown
Contributor

@jasnell it doesn't look like 2930867 has been backported to LTS. I don't think we should be backporting this commit unless we upgrade v8 to 4.6 on LTS

thoughts?

MylesBorins commented Jan 6, 2016

Copy link
Copy Markdown
Contributor

Removing LTS tag. @jasnell @rvagg feel free to re apply

MylesBorins mentioned this pull request Jan 14, 2016
Closed
snyk-bot mentioned this pull request Mar 16, 2020
Open
snyk-bot mentioned this pull request Aug 10, 2020
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL