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: remove extra field from v8::HeapStatistics #7526

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jul 3, 2016

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

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

Ref: nodejs#7016
@addaleax addaleax added v8 engine Issues and PRs related to the V8 dependency. dont-land-on-v5.x labels Jul 3, 2016
@indutny
Copy link
Member

indutny commented Jul 3, 2016

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

@jasnell
Copy link
Member

jasnell commented Jul 3, 2016

Yeah, this should likely be pushed up stream first.

@bnoordhuis
Copy link
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.

@jasnell
Copy link
Member

jasnell commented Jul 3, 2016

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

@addaleax
Copy link
Member Author

addaleax commented Jul 5, 2016

@addaleax
Copy link
Member Author

addaleax commented Jul 7, 2016

Landed in 4c774e1

@addaleax addaleax closed this Jul 7, 2016
@addaleax 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
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 <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Fishrock123
Copy link
Contributor

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

@addaleax
Copy link
Member Author

addaleax commented Jul 7, 2016

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.

@Fishrock123
Copy link
Contributor

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

@targos
Copy link
Member

targos commented Jul 7, 2016

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.

ofrobots pushed a commit to ofrobots/node that referenced this pull request Aug 25, 2016
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 <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
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.

6 participants