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

deps: backport 7dfb5beeec from V8 upstream by MylesBorins · Pull Request #7348 · nodejs/node · GitHub

/ node Public

deps: backport 7dfb5beeec from V8 upstream - #7348

Closed
MylesBorins wants to merge 1 commit into
nodejs:masterfrom
MylesBorins:null-check
ConversationCommits (1)ChecksFiles changed
Closed

deps: backport 7dfb5beeec from V8 upstream#7348
MylesBorins wants to merge 1 commit into
nodejs:masterfrom
MylesBorins:null-check

Conversation

MylesBorins commented Jun 21, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

deps

Description of change

This commit backports a fix to a JIT bug in V8.
After 100 or so comparisons typeof null ==="undefined" is returning
true instead of false.

Original commit message:

Fix 'typeof null' canonicalization in crankshaft

BUG=

Review URL: https://codereview.chromium.org/1912553002

Cr-Commit-Position: refs/heads/master@{#35699}

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Jun 21, 2016

MylesBorins commented Jun 21, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

ci: https://ci.nodejs.org/job/node-test-pull-request/3044/
v8 tests: https://ci.nodejs.org/job/node-test-commit-v8-linux/141/

/cc @ofrobots @nodejs/v8

indutny commented Jun 21, 2016

Copy link
Copy Markdown
Member

Any reason to rush it and not wait until it will be merged into V8 5.2?

MylesBorins commented Jun 21, 2016

Copy link
Copy Markdown
Contributor Author

Based on the reddit thread it seemed like people were running into this problem in production. We can likely wait on it, but I wasn't sure how long it would take to prove out that the V8 5.2 update is non ABI breaking

indutny commented Jun 21, 2016

Copy link
Copy Markdown
Member

@thealphanerd 👍 then.

bnoordhuis commented Jun 21, 2016

Copy link
Copy Markdown
Member

@thealphanerd Can you request a back-port to 5.0 and 5.1 upstream? Then we can just upgrade wholesale.

bnoordhuis commented Jun 21, 2016

Copy link
Copy Markdown
Member

Also, it looks like v6 is affected but v4 and v5 are not.

MylesBorins commented Jun 21, 2016

Copy link
Copy Markdown
Contributor Author

@bnoordhuis more than happy to. What is the process for doing so?

bnoordhuis commented Jun 21, 2016

Copy link
Copy Markdown
Member

@thealphanerd https://github.com/v8/v8/wiki/Merging%20&%20Patching

MylesBorins mentioned this pull request Jun 21, 2016
Closed

MylesBorins commented Jun 21, 2016

Copy link
Copy Markdown
Contributor Author

I have opened a backport request in the chromium tracker

https://codereview.chromium.org/2087803004/

@ofrobots seems to imply that we should likely float for V8 5 until nodejs/Release#111 has been sorted

ofrobots commented Jun 21, 2016

Copy link
Copy Markdown
Contributor

This got cherry-picked onto in V8 5.1 earlier today. As far as V8 5.0 is concerned, that branch is no longer getting tested, and there is no official process to merge changes into it, until nodejs/Release#111 gets to closure. /cc @natorion

Until then I think we should float this patch.

@thealphanerd instead if the reddit link, could you link to the upstream bug for this issue: https://bugs.chromium.org/p/chromium/issues/detail?id=604033.

Otherwise this LGTM as long as V8 tests pass.

bnoordhuis commented Jun 21, 2016

Copy link
Copy Markdown
Member

LGTM but I second Ali's comment about the reddit link.

MylesBorins commented Jun 21, 2016

Copy link
Copy Markdown
Contributor Author

Link updated. In future I'll double check the chromium tracker first 😄

V8 tests passed, just waiting on a fresh run of the CI

MylesBorins self-assigned this Jun 21, 2016

targos commented Jun 21, 2016

Copy link
Copy Markdown
Member

LGTM.
There is a nit in the commit message: s/B8/V8/

deps: backport 7dfb5beeec from V8 upstream
92507bb 
This commit backports a fix to a JIT bug in V8.
After 100 or so comparisons `typeof null ==="undefined"` is returning
`true` instead of `false`.

Original commit message:

	Fix 'typeof null' canonicalization in crankshaft

	BUG=

	Review URL: https://codereview.chromium.org/1912553002

	Cr-Commit-Position: refs/heads/master@{nodejs#35699}

Ref: https://bugs.chromium.org/p/chromium/issues/detail?id=604033

MylesBorins commented Jun 21, 2016

Copy link
Copy Markdown
Contributor Author

thanks @targos I was doing s/v/V and accidentally s/v/B'd

¯_(ツ)_/¯

MylesBorins changed the title deps: backport 7dfb5beeec from v8 upstream deps: backport 7dfb5beeec from V8 upstream Jun 21, 2016
MylesBorins mentioned this pull request Jun 21, 2016
Closed

MylesBorins commented Jun 22, 2016

Copy link
Copy Markdown
Contributor Author

landed in a84b0f2

MylesBorins added a commit that referenced this pull request Jun 22, 2016
deps: backport 7dfb5beeec from V8 upstream
a84b0f2 
This commit backports a fix to a JIT bug in V8.
After 100 or so comparisons `typeof null ==="undefined"` is returning
`true` instead of `false`.

Original commit message:

	Fix 'typeof null' canonicalization in crankshaft

	BUG=

	Review URL: https://codereview.chromium.org/1912553002

	Cr-Commit-Position: refs/heads/master@{#35699}

Ref: https://bugs.chromium.org/p/chromium/issues/detail?id=604033
PR-URL: #7348
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
MylesBorins added a commit that referenced this pull request Jun 22, 2016
deps: backport 7dfb5beeec from V8 upstream
c544213 
This commit backports a fix to a JIT bug in V8.
After 100 or so comparisons `typeof null ==="undefined"` is returning
`true` instead of `false`.

Original commit message:

	Fix 'typeof null' canonicalization in crankshaft

	BUG=

	Review URL: https://codereview.chromium.org/1912553002

	Cr-Commit-Position: refs/heads/master@{#35699}

Ref: https://bugs.chromium.org/p/chromium/issues/detail?id=604033
PR-URL: #7348
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

MylesBorins commented Jun 22, 2016

Copy link
Copy Markdown
Contributor Author

I've gone ahead and landed this on v6.x as c544213

Fishrock123 mentioned this pull request Jun 27, 2016
Merged
Fishrock123 mentioned this pull request Jul 5, 2016
Merged
gibfahn pushed a commit to ibmruntimes/node that referenced this pull request Jul 7, 2016
deps/v8z: backport 7dfb5beeec from V8 upstream
1f5c84c 
This commit backports a fix to a JIT bug in V8.
After 100 or so comparisons `typeof null ==="undefined"` is returning
`true` instead of `false`.

Original commit message:

	Fix 'typeof null' canonicalization in crankshaft

	BUG=

	Review URL: https://codereview.chromium.org/1912553002

	Cr-Commit-Position: refs/heads/master@{#35699}

Ref: https://bugs.chromium.org/p/chromium/issues/detail?id=604033
PR-URL: nodejs/node#7348
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
MylesBorins deleted the null-check branch July 8, 2016 14:53
MylesBorins removed their assignment Dec 27, 2016
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

v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL