GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — deps: remove extra field from v8::HeapStatistics by addaleax · Pull Request #7526 · nodejs/node

Conversation

addaleax commented Jul 3, 2016

Copy link

Copy Markdown

Member

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

deps/v8

Description of change

Remove the _malloced_memory field from the HeapStatistics class to achieve full ABI compatibility with V8 5.0 (as far as I can tell).

This structure is probably not something used by addons anyway, since the information from that struct is already made available through Node’s v8.getHeapStatistics(), but better safe than sorry.

Ref: #7016 (comment) (and it would need to be landed in v6 together with #7016).

/cc @ofrobots @nodejs/v8

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          deps: remove extra field from v8::HeapStatistics

3bec4f0

Remove the `_malloced_memory` field from the `HeapStatistics`
class to achieve full ABI compatibility with V8 5.0.

Ref: nodejs#7016

addaleax added v8 engine

Issues and PRs related to the V8 dependency.

dont-land-on-v5.x labels

Jul 3, 2016

indutny commented Jul 3, 2016

Copy link

Copy Markdown

Member

LGTM, I guess. Any chance V8 would accept this?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell commented Jul 3, 2016

Copy link

Copy Markdown

Member

Yeah, this should likely be pushed up stream first.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnoordhuis commented Jul 3, 2016

Copy link

Copy Markdown

Member

LGTM

@indutny @jasnell This change was supposed to have been part of commit 9beef23 that restores 5.0 API/ABI compatibility. It's a patch we're floating on top of 5.1, it's not something we can (or want to) upstream.

addaleax reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell commented Jul 3, 2016

Copy link

Copy Markdown

Member

@bnoordhuis +1 SGTM!
I withdraw my objection and give a LGTM!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax commented Jul 5, 2016

Copy link

Copy Markdown

Member Author

CI: https://ci.nodejs.org/job/node-test-commit/3971/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax added the dont-land-on-v6.x label

Jul 5, 2016

addaleax commented Jul 7, 2016

Copy link

Copy Markdown

Member Author

Landed in 4c774e1

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax closed this

Jul 7, 2016

addaleax deleted the v8-5.0-abi-compat branch

July 7, 2016 08:36

addaleax added a commit that referenced this pull request

Jul 7, 2016


          deps: remove extra field from v8::HeapStatistics

4c774e1

Remove the `_malloced_memory` field from the `HeapStatistics`
class to achieve full ABI compatibility with V8 5.0.

Ref: #7016
PR-URL: #7526
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Fishrock123 commented Jul 7, 2016

Copy link

Copy Markdown

Contributor

@addaleax Is this supposed to land on v6 but not v7?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax commented Jul 7, 2016

Copy link

Copy Markdown

Member Author

I’ve added the label to match the one you added on #7016; the idea still is to land #7016 and this in v6 once enough people feel comfortable to do so.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fishrock123 commented Jul 7, 2016

Copy link

Copy Markdown

Contributor

@addaleax Correct, but wouldn't we want this new field for V7?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

targos commented Jul 7, 2016

Copy link

Copy Markdown

Member

for v7 we will have a new version of V8. The process of upgrading V8 to a new major version cancels all patches that we apply to deps/v8. We have to decide then which ones we want to reapply.

Fishrock123 and addaleax reacted with thumbs up emoji

All reactions

👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ofrobots pushed a commit to ofrobots/node that referenced this pull request

Aug 25, 2016


          deps: remove extra field from v8::HeapStatistics

9a4b338

Remove the `_malloced_memory` field from the `HeapStatistics`
class to achieve full ABI compatibility with V8 5.0.

Ref: nodejs#7016
PR-URL: nodejs#7526
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>

evanlucas mentioned this pull request

Aug 26, 2016

2016-08-26, Version 6.5.0 (Current) #8253

Merged

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

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.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.