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

[v10.x] backport EmbededderGraph related changes to v10.x #23295

Closed

Conversation

joyeecheung
Copy link
Member

src: name EmbededderGraph edges and use class names for nodes

This patch:

  • Refactors the MemoryRetainer API so that the impementer no longer
    calls TrackThis() that sets the size of node on the top of the
    stack, which may be hard to understand. Instead now they implements
    SelfSize() to provide their self sizes. Also documents
    the API in the header.
  • Refactors MemoryTracker so it calls MemoryInfoName() and
    SelfSize() of MemoryRetainer to retrieve info about them, and
    separate node_names and edge_names so the edges can be properly
    named with reference names and the nodes can be named with class
    names. (Previously the nodes are named with reference names while the
    edges are all indexed and appear as array elements).
  • Adds SET_MEMORY_INFO_NAME(), SET_SELF_SIZE() and
    SET_NO_MEMORY_INFO() convenience macros
  • Fixes a few MemoryInfo calls in some MemoryRetainers to track
    their references properly.
  • Refactors the heapdump tests to check both node names and edge names,
    distinguishing between wrapped JS nodes (without prefixes)
    and embedder wrappers (prefixed with Node / ).

PR-URL: #23072
Reviewed-By: Anna Henningsen anna@addaleax.net

src: implement the new EmbedderGraph::AddEdge()

The signature of EmbedderGraph::AddEdge() has been changed so
the current implementation of JSGraph no longer compiles.
This patch updates the implementation accordingly.

PR-URL: #22106
Refs: v8/v8@6ee8345
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: James M Snell jasnell@gmail.com

The signature of EmbedderGraph::AddEdge() has been changed so
the current implementation of JSGraph no longer compiles.
This patch updates the implementation accordingly.

PR-URL: nodejs#22106
Refs: v8/v8@6ee8345
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
This patch:

- Refactors the `MemoryRetainer` API so that the impementer no longer
  calls `TrackThis()` that sets the size of node on the top of the
  stack, which may be hard to understand. Instead now they implements
  `SelfSize()` to provide their self sizes. Also documents
  the API in the header.
- Refactors `MemoryTracker` so it calls `MemoryInfoName()` and
  `SelfSize()` of `MemoryRetainer` to retrieve info about them, and
  separate `node_names` and `edge_names` so the edges can be properly
  named with reference names and the nodes can be named with class
  names. (Previously the nodes are named with reference names while the
  edges are all indexed and appear as array elements).
- Adds `SET_MEMORY_INFO_NAME()`, `SET_SELF_SIZE()` and
  `SET_NO_MEMORY_INFO()` convenience macros
- Fixes a few `MemoryInfo` calls in some `MemoryRetainers` to track
  their references properly.
- Refactors the heapdump tests to check both node names and edge names,
  distinguishing between wrapped JS nodes (without prefixes)
  and embedder wrappers (prefixed with `Node / `).

PR-URL: nodejs#23072
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@joyeecheung joyeecheung requested a review from addaleax October 6, 2018 19:17
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v10.x labels Oct 6, 2018
@joyeecheung
Copy link
Member Author

These are backported without the ABI breaking V8 commit in #22106 to reduce the amount of churn for future changes. The edge names are just going to be ignored in both the implementation and in the tests.

@joyeecheung
Copy link
Member Author

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the backport! :)

targos pushed a commit that referenced this pull request Oct 7, 2018
The signature of EmbedderGraph::AddEdge() has been changed so
the current implementation of JSGraph no longer compiles.
This patch updates the implementation accordingly.

Backport-PR-URL: #23295
PR-URL: #22106
Refs: v8/v8@6ee8345
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Oct 7, 2018
This patch:

- Refactors the `MemoryRetainer` API so that the impementer no longer
  calls `TrackThis()` that sets the size of node on the top of the
  stack, which may be hard to understand. Instead now they implements
  `SelfSize()` to provide their self sizes. Also documents
  the API in the header.
- Refactors `MemoryTracker` so it calls `MemoryInfoName()` and
  `SelfSize()` of `MemoryRetainer` to retrieve info about them, and
  separate `node_names` and `edge_names` so the edges can be properly
  named with reference names and the nodes can be named with class
  names. (Previously the nodes are named with reference names while the
  edges are all indexed and appear as array elements).
- Adds `SET_MEMORY_INFO_NAME()`, `SET_SELF_SIZE()` and
  `SET_NO_MEMORY_INFO()` convenience macros
- Fixes a few `MemoryInfo` calls in some `MemoryRetainers` to track
  their references properly.
- Refactors the heapdump tests to check both node names and edge names,
  distinguishing between wrapped JS nodes (without prefixes)
  and embedder wrappers (prefixed with `Node / `).

Backport-PR-URL: #23295
PR-URL: #23072
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@targos
Copy link
Member

targos commented Oct 7, 2018

Landed in 1712458 and 8fff3b0

@targos targos closed this Oct 7, 2018
targos pushed a commit that referenced this pull request Oct 7, 2018
The signature of EmbedderGraph::AddEdge() has been changed so
the current implementation of JSGraph no longer compiles.
This patch updates the implementation accordingly.

Backport-PR-URL: #23295
PR-URL: #22106
Refs: v8/v8@6ee8345
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Oct 7, 2018
This patch:

- Refactors the `MemoryRetainer` API so that the impementer no longer
  calls `TrackThis()` that sets the size of node on the top of the
  stack, which may be hard to understand. Instead now they implements
  `SelfSize()` to provide their self sizes. Also documents
  the API in the header.
- Refactors `MemoryTracker` so it calls `MemoryInfoName()` and
  `SelfSize()` of `MemoryRetainer` to retrieve info about them, and
  separate `node_names` and `edge_names` so the edges can be properly
  named with reference names and the nodes can be named with class
  names. (Previously the nodes are named with reference names while the
  edges are all indexed and appear as array elements).
- Adds `SET_MEMORY_INFO_NAME()`, `SET_SELF_SIZE()` and
  `SET_NO_MEMORY_INFO()` convenience macros
- Fixes a few `MemoryInfo` calls in some `MemoryRetainers` to track
  their references properly.
- Refactors the heapdump tests to check both node names and edge names,
  distinguishing between wrapped JS nodes (without prefixes)
  and embedder wrappers (prefixed with `Node / `).

Backport-PR-URL: #23295
PR-URL: #23072
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants