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

deps: backport 2bcbe2f from V8 upstream #7814

Merged
merged 1 commit into from
Jul 29, 2016

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Jul 21, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change
Excessive buffering of perf map files in V8 could cause profiles
to be missing symbols at times.

Original commit message:
  switch perf and ll_prof loggers to line buffering

  BUG=v8:5015
  [email protected],[email protected]

  Review-Url: https://codereview.chromium.org/2041243002
  Cr-Commit-Position: refs/heads/master@{#36788}

Note that this is not needed on master as V8 5.1 already contains this fix.
R=@nodejs/v8

EDIT: CI: https://ci.nodejs.org/job/node-test-pull-request/3362/

@ofrobots ofrobots added v8 engine Issues and PRs related to the V8 dependency. lts-watch-v4.x labels Jul 21, 2016
@mscdex mscdex added the v6.x label Jul 21, 2016
@bnoordhuis
Copy link
Member

LGTM but out of curiosity, is there a reason you opted for line buffering instead of no buffering? It seems the output is line oriented anyways so libc's newline scanning just adds unnecessary, if minimal, overhead.

@ofrobots
Copy link
Contributor Author

No strong reason other than to avoid any file watchers from observing partially complete lines. The output is indeed line oriented today; but the code may change in the future. The new line scanning is going to be negligible cost.

@ofrobots
Copy link
Contributor Author

Rebased, added a V8 patch-level bump. Relaunched CI: https://ci.nodejs.org/job/node-test-pull-request/3413/.

@ofrobots
Copy link
Contributor Author

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/244/. I think the Power BE machine has infrastructure issues. The rest are green.

@ofrobots
Copy link
Contributor Author

@nodejs/release: this is ready for landing onto v6.x. Should I do that myself?

@evanlucas
Copy link
Contributor

works for me :]

Excessive buffering of perf map files in V8 could cause profiles
to be missing symbols at times.

Original commit message:
  switch perf and ll_prof loggers to line buffering

  BUG=v8:5015
  [email protected],[email protected]

  Review-Url: https://codereview.chromium.org/2041243002
  Cr-Commit-Position: refs/heads/master@{nodejs#36788}

PR-URL: nodejs#7814
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
@ofrobots ofrobots merged commit e82e804 into nodejs:v6.x Jul 29, 2016
@ofrobots
Copy link
Contributor Author

Thanks. Landed on v6.x as e82e804.

@cjihrig cjihrig mentioned this pull request Aug 11, 2016
@ofrobots ofrobots deleted the perf-buffering-6.x branch August 17, 2016 17:58
BethGriggs pushed a commit to ibmruntimes/node that referenced this pull request Aug 18, 2016
Excessive buffering of perf map files in V8 could cause profiles
to be missing symbols at times.

Original commit message:
  switch perf and ll_prof loggers to line buffering

  BUG=v8:5015
  [email protected],[email protected]

  Review-Url: https://codereview.chromium.org/2041243002
  Cr-Commit-Position: refs/heads/master@{#36788}

PR-URL: nodejs/node#7814
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
Excessive buffering of perf map files in V8 could cause profiles
to be missing symbols at times.

Original commit message:
  switch perf and ll_prof loggers to line buffering

  BUG=v8:5015
  [email protected],[email protected]

  Review-Url: https://codereview.chromium.org/2041243002
  Cr-Commit-Position: refs/heads/master@{#36788}

PR-URL: #7814
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
Excessive buffering of perf map files in V8 could cause profiles
to be missing symbols at times.

Original commit message:
  switch perf and ll_prof loggers to line buffering

  BUG=v8:5015
  [email protected],[email protected]

  Review-Url: https://codereview.chromium.org/2041243002
  Cr-Commit-Position: refs/heads/master@{#36788}

PR-URL: #7814
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
Excessive buffering of perf map files in V8 could cause profiles
to be missing symbols at times.

Original commit message:
  switch perf and ll_prof loggers to line buffering

  BUG=v8:5015
  [email protected],[email protected]

  Review-Url: https://codereview.chromium.org/2041243002
  Cr-Commit-Position: refs/heads/master@{#36788}

PR-URL: #7814
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
BethGriggs pushed a commit to ibmruntimes/node that referenced this pull request Nov 9, 2016
Excessive buffering of perf map files in V8 could cause profiles
to be missing symbols at times.

Original commit message:
  switch perf and ll_prof loggers to line buffering

  BUG=v8:5015
  [email protected],[email protected]

  Review-Url: https://codereview.chromium.org/2041243002
  Cr-Commit-Position: refs/heads/master@{#36788}

PR-URL: nodejs/node#7814
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants