Introduce BuildEmbedderGraphCallback

[danbev]

• Version: v11.0.0-pre
• Platform: Darwin ovpn-116-59.ams2.redhat.com 15.6.0 Darwin Kernel Version 15.6.0: Tue Jan 30 11:45:51 PST 2018; root:xnu-3248.73.8~1/RELEASE_X86_64 x86_64
• Subsystem: src

Currently when building node there are a number of warnings like this one:

./src/async_wrap.cc:611:29: warning: 'SetWrapperClassInfoProvider' is deprecated [-Wdeprecated-declarations]
  NODE_ASYNC_PROVIDER_TYPES(V)

In deps/v8/include/v8-profiler.h we can find the following deprecation :

V8_DEPRECATED(
      "Use SetBuildEmbedderGraphCallback to provide info about embedder nodes",
      void SetWrapperClassInfoProvider(uint16_t class_id,
                                       WrapperInfoCallback callback));

The goal of this issue is to start using SetBuildEmbedderGraphCallback instead of the now deprecated SetWrapperClassInfoProvider.

I've taken a look at the unit tests in V8, and tried to follow what they do in this branch: embedder-graph-builder.
This is still a work in progress, but I wanted to get some feedback, and also I'll be out for about a month now so if anyone would like to pick this up then feel free.

[targos]

Did you mean to open a PR?

[danbev]

Did you mean to open a PR?

No, I actually meant to open this as an issue. I considered opening it as a PR but since I'll be out for the next month it might just probably just sit there without me being able to respond. I'm happy to open a PR instead if you prefer?

[targos]

No it's fine, my bad. I didn't check your link because I thought it was pointing to V8's unit tests.

[addaleax]

Sorry for not advertising it so far, but I also have some (WIP but pretty close, imo) commits in https://github.com/addaleax/node/tree/heap – it fully should fully address the deprecation and generally improves heap dump tracking a lot. (As in: Our current accuracy for memory retention reporting is … bad.)

It’s currently still be blocked on #21608, though – not strictly a necessity, but that would be nice to have before addressing this.

It’s nice to see that @danbev’s branch has testing for the snapshot content itself. What I was going for with the last commit in my branch (addaleax/node@147e687) is exposing an internal utility that provides the same information that we’d give V8 for a heap dump, just as JS objects, and testing against that; the nice thing here is that we could write JS tests for this. Maybe there’s a way to make both approaches work?

[danbev]

@addaleax Great to hear that you have this in progress. Should I close this issue or just update the description and you can ref it in your PR later?

[addaleax]

@danbev Let’s keep this issue open – at the very least, we probably want to look into your snapshot testing changes?