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

Add cppgc bindings #1336

Merged
merged 17 commits into from
Oct 30, 2023
Merged

Add cppgc bindings #1336

merged 17 commits into from
Oct 30, 2023

Conversation

littledivy
Copy link
Member

@littledivy littledivy commented Sep 28, 2023

https://v8.dev/blog/high-performance-cpp-gc

Oilpan in Deno design doc: https://www.notion.so/denolandinc/Oilpan-cppgc-in-Deno-e194f4268e9f4135ba97610ff7d3a949?pvs=4

Oilpan can be used to implement GC'able resources in Deno

Closes #933

@littledivy littledivy changed the title Add cppgc bindings POC: Add cppgc bindings Sep 28, 2023
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Will need a layer ontop in deno_core to make more rust idiomatic I think. Looking good

@mmastrac
Copy link
Contributor

First of all, this is awesome!

I wonder if we should keep this interface low-level for now. There's a few Gc container types in Rust already and we may want to experiment with different designs until we find something that's ergonomic for what we need.

https://manishearth.github.io/blog/2021/04/05/a-tour-of-safe-tracing-gc-designs-in-rust/

I think the ideal level of interface here might be the ability to create a Boxed T with the correct C++ vtable that offers the low-level tracing API (unopinionated at this level), and automatically drops its inner object when the GC finds it goes out of scope.

What we'd then do is build a library (maybe in the deno_core project?) to offer a Gc<T> type based on the low-level interface.

@littledivy littledivy changed the title POC: Add cppgc bindings Add cppgc bindings Sep 29, 2023
@littledivy littledivy marked this pull request as ready for review September 29, 2023 05:36
/// Can be called multiple times when paired with `ShutdownProcess()`.
pub fn initalize_process(platform: SharedRef<Platform>) {
unsafe {
cppgc__initialize_process(&*platform as *const Platform as *mut _);
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: &mut *platform as *mut Platform perhaps? Or does SharedRef possibly have an as_ptr or as_mut_ptr API?

Copy link
Member Author

Choose a reason for hiding this comment

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

SharedRef does not deref as mutable. This is a common pattern for passing pointers where Rust owernship does not matter:

v8__V8__InitializePlatform(&*platform as *const Platform as *mut _)

Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this PR, but we should probably have as_ptr and as_mut_ptr convenience methods on SharedRef to avoid the need to deref/ref and cast to pointer.

Copy link
Contributor

@mmastrac mmastrac left a comment

Choose a reason for hiding this comment

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

LGTM!

@littledivy littledivy merged commit 7072da4 into denoland:main Oct 30, 2023
@littledivy littledivy deleted the oilpan branch October 30, 2023 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

V8 EmbedderHeapTracer
4 participants