[v8.x] Fix abi break #24499
Closed
[v8.x] Fix abi break #24499
Conversation
This file contains 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
d868eb7 ended up being an inadvertant ABI change. Remove the call to CheckMemoryPressure from the header itself.
|
@ofrobots sadly an error occured when I tried to trigger a build :( |
nodejs-github-bot
added
c++
2 tasks
CheckMemoryPressure cannot be used ABI-safely from v8.h. Add a alternate implementation of AdjustAmountOfExternalAllocatedMemory and then use that from Node.
ofrobots
force-pushed
the
fix-abi-break
branch
from
5cbf4c9
to
dc1bd96
Compare
addaleax
approved these changes
Nov 20, 2018
hashseed
approved these changes
Nov 20, 2018
|
V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1892/ I think we should rush this into today's release if the CI is green /cc @BethGriggs |
Merged
|
Both CI jobs are ✔️ |
MylesBorins
pushed a commit
that referenced
this pull request
Nov 20, 2018
d868eb7 ended up being an inadvertant ABI change. Remove the call to CheckMemoryPressure from the header itself. PR-URL: #24499 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins
pushed a commit
that referenced
this pull request
Nov 20, 2018
CheckMemoryPressure cannot be used ABI-safely from v8.h. Add a alternate implementation of AdjustAmountOfExternalAllocatedMemory and then use that from Node. PR-URL: #24499 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Myles Borins <myles.borins@gmail.com>
|
landed in 62dd1d7...0157e3e |
|
The fix should be safe enough include in today's release.
…On Tue, Nov 20, 2018, 7:04 AM Myles Borins ***@***.*** wrote:
***@***.**** approved this pull request.
LGTM
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#24499 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAE0qTIVjxxfCVwOsJgB9J0BZ5N5cetIks5uxBnzgaJpZM4YqMDc>
.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses the inadvertent ABI-incompatible change in #21269. The first commit reverts the ABI breaking part of d868eb7. Unfortunately doing so
willmay reintroduce a memory bug (#21021). This is where the second commit adds back the functionality in an ABI-safe way and then uses it from within core.Native modules that use
AdjustAmountOfExternalAllocatedMemorymay still miss theCheckMemoryPressurepath. This is unfortunate, but there is no easy way to do so in an ABI compatible way. Going forward, it would be good if V8 was to avoid implementation in thev8.hfile as that can lead to ABI issues like this where we can't fix the memory issue in an ABI compatible way. Such modules can use the 8.x specificAdjustAmountOfExternalAllocatedMemoryCustom, but would require a recompilation.make -j4 test(UNIX), orvcbuild test(Windows) passesCI: https://ci.nodejs.org/job/node-test-pull-request/18778/
V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1887/