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 Gc::as_ptr by analogy with Rc::as_ptr #172

Closed
anna-hope opened this issue Oct 3, 2023 · 6 comments · Fixed by #173
Closed

Add Gc::as_ptr by analogy with Rc::as_ptr #172

anna-hope opened this issue Oct 3, 2023 · 6 comments · Fixed by #173

Comments

@anna-hope
Copy link
Contributor

anna-hope commented Oct 3, 2023

For context, I'm working on a bytecode interpreter for the Lox language. In my implementation, I'm currently using Rc, but I ultimately would like to use a real GC instead.

There is a part of my code where I rely on Rc::as_ptr to get the raw pointer in order to compare memory addresses, roughly like this:

if Rc::as_ptr(&x) > Rc::as_ptr(&y) {
    // Do stuff.
}

I need this to check which value was older when traversing a linked list.

I didn't find a similar associated function for Gc, and I don't know of another way of accomplishing this without it.

It might also be useful to have for other purposes.

If we agree that this would be a useful function to have, I would be happy to try implementing it.

@Manishearth
Copy link
Owner

Yeah I'd accept a PR adding this

However, please note that pointer comparison does not guarantee a pointer is "older"

@andersk
Copy link
Collaborator

andersk commented Oct 3, 2023

I didn't find a similar associated function for Gc, and I don't know of another way of accomplishing this without it.

Gc::as_ptr is a fine addition, but for the record, you can cast any reference to a pointer in safe Rust (my_gc.as_ref() as *const _ or &*my_gc as *const _). One might want to avoid that path in some cases if creating the reference itself would be undefined behavior (if, say, the pointer might be null or misaligned or the wrong type), but that’s not a concern here.

And to second Manishearth’s warning: Rust does not allocate memory in increasing address order, since for example freed memory will be reused.

@anna-hope
Copy link
Contributor Author

@andersk Thanks for pointing that out! I'd forgotten about casting references to pointers. I don't often use as in Rust when there are alternatives, but you're right, that would likely work here.

As for the warning on memory address order -- I'll note that. If you're curious, the actual code is here and here. It's a pretty straight port of C code from the book, though it seems C doesn't guarantee memory address order either. Perhaps there is something else to this logic that I'm not understanding, because it works in my implementation (and obviously in the C code from the book) -- unless I'm not using complex enough test cases.

@Manishearth I've submitted a PR with the addition.

@Manishearth
Copy link
Owner

Manishearth commented Oct 3, 2023

If you're implementing a GC from a book I suspect the book is providing a deliberately naïve implementation.

because it works in my implementation (and obviously in the C code from the book) -- unless I'm not using complex enough test cases

Yes, it'll take a while for an allocator to eventually cycle around here.

@anna-hope
Copy link
Contributor Author

If you're implementing a GC from a book I suspect the book is providing a deliberately naïve implementation.

To clarify, it's the implementation of closing upvalues for closures that uses the pointer comparison.

The book provides its own implementation of a GC, but I am not attempting that just yet, which is why I wanted to use this crate. Anyway, thanks for merging the PR so quickly!

@andersk
Copy link
Collaborator

andersk commented Oct 3, 2023

I haven’t tried to read your code, but depending on its implementation details, a compacting garbage collector may be able to maintain a guarantee that addresses are ordered chronologically, with the tradeoff that the addresses change during compaction. (This library is not compacting.)

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 a pull request may close this issue.

3 participants