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

inspector: support non-V8 dispatchers #7393

Closed
joshgav opened this issue Jun 23, 2016 · 6 comments
Closed

inspector: support non-V8 dispatchers #7393

joshgav opened this issue Jun 23, 2016 · 6 comments
Labels
discuss Issues opened for discussions and feedbacks. inspector Issues and PRs related to the V8 inspector protocol

Comments

@joshgav
Copy link
Contributor

joshgav commented Jun 23, 2016

Background

In #7266 (comment) we began discussing separating the Inspector Protocol front-end from the V8-specific backend implementation in Google's v8_inspector contribution.

I believe we should do this so that the same Inspector Agent and Protocol Gateway in Node.js work with multiple backend implementations, starting with V8 Inspector. This would let other VM vendors create backends optimized for their APIs and capabilities, and would facilitate others adding new capabilities to the protocol through their own implementations.

If we don't do this other VMs would have to provide an alternate agent, gateway, and backend implementation for their diagnostic system; or provide (yet another) shim to V8's C++ diagnostic APIs.

Proof of Concept

To prove the concept, I implemented most of the separation in just this small commit: joshgav@5f2507d. The main changes are:

  • Add Inspector.h|cpp to ./deps/v8_inspector/platform/inspector_protocol as base class for all backend implementations, starting with V8Inspector in ./deps/v8_inspector/platform/v8_inspector/public/V8Inspector.h.
  • Refactor v8_inspector.gyp file into two - one for generating and compiling the protocol frontend and one for the backend implementation.
  • Configure ./src/inspector_agent.h|cc to dynamically load the correct backend implementation.
    • Note: Marked with a TODO because there are several ways to do this.

So, @nodejs/collaborators and @pavelfeldman:

  • In theory, what do you think of the proposal?
  • In practice, how can we implement this in cooperation with upstream?

One option could be to create an "inspector-gateway" repo in the nodejs org and maintain the protocol frontend there as an open project. We'd then include it in ./deps like other upstream dependencies.


Related

/cc @nodejs/diagnostics @thealphanerd @ofrobots @pavelfeldman @nodejs/chakracore @ehsan

Edits:
edit (2016-06-24): fix issue title to better match conventions

@pavelfeldman
Copy link
Contributor

In practice, how can we implement this in cooperation with upstream?

Overall, given that the protocol format is stable and the code generator is only a 2KLOC, cloning it for the sake of making progress would also work. Reusing some of it also makes sense and we are trying to land it as v8's dependency that embedder can control (similarly to ICU). But there are too many moving parts for us as we are moving v8_inspector from Blink to V8, so we'd rather let the dust settle before we think about it.

@mscdex mscdex added inspector Issues and PRs related to the V8 inspector protocol discuss Issues opened for discussions and feedbacks. labels Jun 23, 2016
@joshgav joshgav changed the title Proposal: Separate Inspector Protocol Gateway from V8-specific backend implementation inspector: proposal: separate Inspector Protocol gateway from V8 backend implementation Jun 24, 2016
@joshgav joshgav changed the title inspector: proposal: separate Inspector Protocol gateway from V8 backend implementation inspector: proposal: separate Inspector protocol gateway from V8 backend implementation Jun 24, 2016
@rvagg
Copy link
Member

rvagg commented Jun 27, 2016

Sounds good in theory. It doesn't seem like there's a big delta here so it's probably only worth it if most of it can be handled upstream.

@joshgav
Copy link
Contributor Author

joshgav commented Jun 29, 2016

Thanks @pavelfeldman and @rvagg, glad you like the idea in theory. Good point that the codebase is small and we could do this when we're ready without too much complication.

Perhaps this would facilitate suggestions like microsoft/language-server-protocol#25?

In the meantime how about I'll continue to monitor PRs and we all keep this in mind as the inspector component matures? @pavelfeldman any chance you can share a timeline?

/cc @nojvek

@pavelfeldman
Copy link
Contributor

any chance you can share a timeline

Back from vacations, let me get back to you when I am up to speed with where we are.

@joshgav joshgav changed the title inspector: proposal: separate Inspector protocol gateway from V8 backend implementation inspector: proposal: separate Inspector protocol gateway to support multiple JS VMs Oct 5, 2016
@joshgav joshgav changed the title inspector: proposal: separate Inspector protocol gateway to support multiple JS VMs inspector: proposal: separate protocol gateway to support multiple JS VMs Oct 5, 2016
@joshgav joshgav changed the title inspector: proposal: separate protocol gateway to support multiple JS VMs inspector: support non-V8-specific dispatchers Feb 27, 2017
@joshgav joshgav changed the title inspector: support non-V8-specific dispatchers inspector: support non-V8 dispatchers Feb 27, 2017
@jasnell
Copy link
Member

jasnell commented May 30, 2017

ping @joshgav .. any progress on this?

@joshgav
Copy link
Contributor Author

joshgav commented May 30, 2017

the implementation I proposed here is no longer feasible so I'll close this issue.

related discussions on Inspector extensibility continue in the Diag WG, particularly in nodejs/diagnostics#75 and nodejs/diagnostics#77

@joshgav joshgav closed this as completed May 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

No branches or pull requests

5 participants