GC cleanup #1654
Open
GC cleanup #1654
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
put those individual headers to work
that VM stuff for stack allocating objects is neat and all but unused for the forseeable future, so away it goes. Also this removes direct references to cons sizes outside of the GC code. Trying to present some kind of coherent API here...
We only ever use one allocator, so the generality is pointless. Even if we had multiple allocators, we'd want a template template since the value type is fixed. With allocators out of the way, one layer of classes turns out to be useless, so away it goes.
cut out some unused code mostly
No idea what this code was about.
I will set up an internal API. watch me
Honestly not sure how much the compiler cares about trivial copyability but who cares, it works
We can destructors for Lisp objects in I think just two situations: 1) When finalizing objects that were allocated on the GC heap. 2) Implicitly when a StackAllocate is destroyed. In both situations the direct class of the object is known, so there's no need for the destructor to be virtual. I think this "forcing a nontrivial destructor" thing was cargo culted at some point. I don't think there's any reason to have a non virtual, non trivial, empty destructor, unless you want to fool is_trivially_destructible. But why would you want that?
again, why not. Also, remove the "explicit"s since they only make sense for converting constructors anyway.
i'm guessing this was supposed to provide a compile time iterator over a class's direct bases, but we have only one base in all but one case (funcallable instance) anyway.
saves a runtime branch in cases where an initializer has to run
I don't see any reason not to just use fill directly. The commit history doesn't make the justification clear. If it's supposed to be for correctness, I don't see anything in cppreference that would make going through a constructor more correct. I think the correct thing would be for all objects to be trivially copyable, but I don't think it's practically a problem even if they aren't. (Which is good, because we have nontrivial destructors.)
These can be implicitly defined now. Also, even if they did have to be explicitly defined, we can do "= default" and keep things trivially constructible.
I accidentally broke it by deleting this seemingly worthless function. Hopefully a comment will stave me off in the future.
Make it accept any callable rather than specifically a function pointer. Also remove the force general root thing that I'm pretty sure is unused, given we only treat the Lisp_O and some symbols as roots, and they're all actually general. And the index parameter which is never actually used and I don't know what it could possibly be good for anyway.
The main thing is we have it only scan one object. The loop thing is how MPS does it but does not seem to be common in GCs, insofar as GCs have any commonality. If you want to do a loop you can repeatedly call the scan function, since it returns a pointer just past the object it scanned, now. This also removes some skip calls in snapshot save and room. It might speed them up a little, but probably not too much.
hasn't actually been in the build for a while, MPS build is dead, and isn't really very informative about anything but MPS's idiosyncracies regardless
This variable was only ever set to one thing anyway, so lose it.
Using a generic memory walker instead of having all the specifics of gatherObjects. gatherObjects is still used elsewhere and is next on the chopping block. This also removes the testing capabilities of ROOM. I think those are better placed in a separate function using the same memory walking tools.
Might need to soup it up to record where the bad objects are accessible from? Not sure.
whatever this was, I don't think it needs to be boehm specific. We could implement something like sb-ext:search-roots with the object mapper.
and run static analyzer, which incidentally catches up with some cando changes. The basic change is that Lisp is no longer polymorphic, which seems to be ok.
Since we have this "garbage collector" thing. Mostly the nontrivial destructor is a bit of a pain. This code has not been touched since the initial commit. Speaking of, delete some allocator constructor/destructors that are never actually used as well.
std::filesystem::path doesn't necessarily have a trivial destructor
also delete some commented out code and some checks. (If we end up needing those checks they should be earlier when the finalizers are installed.)
replacing {} with default makes it trivial which is nice. As for
the dtors, from the boehm documentation, unregistration of a
disappearing link happens implicitly when it's deallocated, so I
don't think we need to bother with it. I also don't think these
destructors were ever actually called, so it's a good thing, too.
dunno what good it is to tag them, they're not exposed to Lisp anyway, and even if they were it's pretty fast to tag
not sure what this was, don't care, the name is unhelpful
it's scan code anyway. should stay in weak_scan.cc where it belongs
unnecessary overhead compared to lambas.
Seems like it would pop up way too much output and/or be specific to whatever particular problem engendered it. I saw at least one recursive lock problem due to the log using dump(), also.
the SharedLockable requirements. This _should_ let us use std::shared_lock and stuff, but I haven't worked that out just yet.
Since it seems to be a continuing issue with some of the extensions
The test fails in FASO builds - there's a "LOCAL-FUN" somewhere that isn't dladdrable. Honestly I have no idea what's going on with that, but snapshots seem to be saved anyway.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Various miscellaneous cleanup of the garbage collection/memory infrastructure as I prep to use Whippet. Shouldn't have any real externally visible changes except that ROOM doesn't test dladdr or output some information about classes. More to come, but I want to merge this before embarking on what might end up being a pretty big rewrite of hash tables.