[Merged by Bors] - Boa Gc implementation draft #2394
Closed
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
nekevss
force-pushed
the
implement-boa-gc
branch
from
e5b649e
to
48d1799
Compare
Razican
requested review from
Razican,
jasonwilliams,
HalidOdat,
RageKnify,
raskad and
jedel1043
Codecov Report
@@ Coverage Diff @@
## main #2394 +/- ##
===========================================
+ Coverage 38.81% 52.34% +13.52%
===========================================
Files 315 329 +14
Lines 24085 34982 +10897
===========================================
+ Hits 9348 18310 +8962
- Misses 14737 16672 +1935
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
I decided to just go ahead and fully integrate it into the engine. Commits are a bit of a monstrosity, but the above appears to be roughly working. Looks like there's some EcmaScript tests that I may have to take a look at, but let me know what you think 😄 |
|
Just a small suggestion. Could you remove the merged commits from main, then rebase? It would make reviewing the code a bit easier, since right now it's grouping your changes with all the merged commits from the main branch. |
jedel1043
added
enhancement
nekevss
force-pushed
the
implement-boa-gc
branch
from
1e1d4b8
to
c11b6f0
Compare
|
Okay, I believe it's just about all cleaned up |
jedel1043
reviewed
Nov 5, 2022
boa_gc/src/lib.rs
Outdated
Comment on lines
33
to
43
| // TODO: Determine if thread local variables are the correct approach vs an initialized structure | ||
| thread_local!(pub static EPHEMERON_QUEUE: StdCell<Option<Vec<GcPointer>>> = StdCell::new(None)); | ||
| thread_local!(pub static GC_DROPPING: StdCell<bool> = StdCell::new(false)); | ||
| thread_local!(static BOA_GC: StdRefCell<BoaGc> = StdRefCell::new( BoaGc { | ||
| config: GcConfig::default(), | ||
| runtime: GcRuntimeData::default(), | ||
| adult_start: StdCell::new(None), | ||
| youth_start: StdCell::new(None), | ||
| })); |
There was a problem hiding this comment.
What do you mean by "initialized structure"? Like, putting everything on a single thread_local!?
There was a problem hiding this comment.
No, probably not the right way to word what I was thinking. I meant a non-thread local struct. Basically, I was thinking something that would have to be initialized by context when I wrote that comment.
Edit: to expand on the above. When working on this, I was mostly thinking of it as a stepping stone away from using rust-gc to an independent garbage collector. I'm not sure how well using thread_local variables play with concurrent collection and I think there was also a question about no_std on an issue, so I was mostly just notating it as something to consider. Maybe it would be better to have left the comment as NOTE: vs. TODO:
There was a problem hiding this comment.
I think we can use thread locals for our std implementation, since they should be more performant on multi-threaded contexts (multiple Gcs can be run at the same time, because JS engines must always run on a single thread). On the other hand, we can use other mechanisms for no_std builds, since a system without an OS has no notion of threads.
nekevss
force-pushed
the
implement-boa-gc
branch
from
4668664
to
a6f52ad
Compare
|
So, I experimented with this PR a bit and it seems like it has a memory leak somewhere? For example, if I run: systemd-run --scope -p MemoryMax=2G --user cargo run --release --bin boa_tester -- runThe memory of the
Compare that to the main branch, which uses 300 MB of memory at max:
I'll try to run this build with Miri, maybe I can find the leak. |
Yeah I think so too. The 262 ci currently exists with code 137 which is probably the job being OOM'd. |
|
Yeah, there's definitely something. I've been looking at it all afternoon. Thought it might have to do with either |
nekevss
force-pushed
the
implement-boa-gc
branch
from
d83b448
to
20d97e7
Compare
|
Just for the record. I ran test builtins::array::tests::array_entries_simple ... error: Undefined Behavior: pointer to alloc1059445 was dereferenced after this allocation got freed
--> /home/jedel/SDK/src/Rust/boa/boa_gc/src/pointers/gc_ptr.rs:84:18
|
84 | unsafe { &*self.inner_ptr() }
| ^^^^^^^^^^^^^^^^^^ pointer to alloc1059445 was dereferenced after this allocation got freed
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= note: BACKTRACE:
= note: inside `boa_gc::Gc::<boa_gc::Cell<object::Object>>::inner` at /home/jedel/SDK/src/Rust/boa/boa_gc/src/pointers/gc_ptr.rs:84:18
note: inside `<boa_gc::Gc<boa_gc::Cell<object::Object>> as boa_gc::Trace>::root` at /home/jedel/SDK/src/Rust/boa/boa_gc/src/pointers/gc_ptr.rs:113:9
--> /home/jedel/SDK/src/Rust/boa/boa_gc/src/pointers/gc_ptr.rs:113:9
|
113 | self.inner().root_inner();
| ^^^^^^^^^^^^
note: inside `object::jsobject::_DERIVE_boa_gc_Trace_FOR_JsObject::<impl boa_gc::Trace for object::jsobject::JsObject>::root::mark::<boa_gc::Gc<boa_gc::Cell<object::Object>>>` at boa_engine/src/object/jsobject.rs:30:10
--> boa_engine/src/object/jsobject.rs:30:10I'll try to come up with a minimized script that replicates the UB. |
|
Ok, I found a pretty good minimization of the problem. A simple script as: use boa_engine::{
object::ObjectData,
prelude::JsObject,
property::PropertyDescriptor,
};
fn main() {
let object = JsObject::from_proto_and_data(None, ObjectData::ordinary());
for i in 0..29 {
let function = JsObject::from_proto_and_data(None, ObjectData::ordinary());
object.borrow_mut().insert(
i,
PropertyDescriptor::builder()
.value(function)
.writable(true)
.enumerable(false)
.configurable(true),
);
}
}Throws a It is very interesting, because the program runs fine if you only insert 27 newly created objects. It is after you try to insert the 28th object when the gc corrupts the memory. |
|
Minimized the problem even further: use boa_gc::BoaAlloc;
fn main() {
let v = BoaAlloc::new_cell(Vec::new());
for _ in 1..=259 {
let cell = BoaAlloc::new_cell([0u8;10]);
v.borrow_mut().push(cell);
}
}This throws with |
|
$ RUSTFLAGS="-Zsanitizer=memory" cargo -Zbuild-std run --target=x86_64-unknown-linux-gnu --bin gc_errorMSAN crashASAN shows the error you got as well, but I think this is caused by a previous memory error. $ RUSTFLAGS="-Zsanitizer=address" cargo -Zbuild-std run --target=x86_64-unknown-linux-gnu --bin gc_errorASAN crash |
nekevss
force-pushed
the
implement-boa-gc
branch
2 times, most recently
from
3cca25a
to
1b04319
Compare
Razican
reviewed
Nov 11, 2022
boa_gc/src/internals/gc_box.rs
Outdated
Comment on lines
80
to
82
| // NOTE: [repr(C)] is most likely unneeded here, but will keep it for now | ||
| /// A garbage collected allocation. | ||
| #[repr(C)] |
There was a problem hiding this comment.
Can we try this out without the #[repr(C)]?
There was a problem hiding this comment.
Yeah, we should be fine. There wasn't anything implemented with layout that would need it.
boa_gc/src/internals/gc_box.rs
Outdated
|
|
||
| impl<T: Trace> GcBox<T> { | ||
| pub(crate) fn new(value: T) -> Self { | ||
| GcBox { |
There was a problem hiding this comment.
Suggested change
| GcBox { | |
| Self { |
This can in fact be done in many places, and can make renames easier at some point.
| } | ||
|
|
||
| /// Increases the root count on this `GcBox`. | ||
| /// Roots prevent the `GcBox` from being destroyed by the garbage collector. |
There was a problem hiding this comment.
Suggested change
| /// Roots prevent the `GcBox` from being destroyed by the garbage collector. | |
| /// | |
| /// Roots prevent the `GcBox` from being destroyed by the garbage collector. |
| &self.value | ||
| } | ||
|
|
||
| pub(crate) fn is_marked(&self) -> bool { |
There was a problem hiding this comment.
This function is missing some documentation, and probably an #[inline] hint. I'm marking this one, since it's the only one missing the documentation in this file, but many other functions / structures / enums are missing documentation. Also, we should probably add the #[inline] hint to one-liners here and elsewhere.
|
The |
RageKnify
reviewed
Nov 13, 2022
Razican
reviewed
Nov 13, 2022
| } | ||
|
|
||
| /// A garbage collected allocation. | ||
| pub(crate) struct GcBox<T: Trace + ?Sized + 'static> { |
There was a problem hiding this comment.
This can probably derive Debug :)
boa_gc/src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| #[derive(Default)] |
There was a problem hiding this comment.
This can probably also derive Debug, Clone and Copy
| bytes_allocated: usize, | ||
| } | ||
|
|
||
| struct BoaGc { |
There was a problem hiding this comment.
This should also implement Debug
boa_gc/src/lib.rs
Outdated
Comment on lines
111
to
114
| // Whether or not the thread is currently in the sweep phase of garbage collection. | ||
| // During this phase, attempts to dereference a `Gc<T>` pointer will trigger a panic. | ||
|
|
||
| struct DropGuard; |
There was a problem hiding this comment.
These comments seem to be together, right? Should they be doc comments related to the struct?
Also, the guard should derive / implement Debug, and maybe Clone/Copy.
boa_gc/src/lib.rs
Outdated
| struct DropGuard; | ||
|
|
||
| impl DropGuard { | ||
| fn new() -> DropGuard { |
There was a problem hiding this comment.
This is missing some docs, with information on the default value. Also, it can return Self, to make it easier to rename.
|
|
||
| /// Returns `true` if it is safe for a type to run [`Finalize::finalize`]. | ||
| #[must_use] | ||
| pub fn finalizer_safe() -> bool { |
There was a problem hiding this comment.
This should have an #[inline] hint.
| /// The Allocator handles allocation of garbage collected values. | ||
| /// | ||
| /// The allocator can trigger a garbage collection. | ||
| struct Allocator; |
There was a problem hiding this comment.
This should at least derive/implement Debug, and maybe Clone/Copy.
raskad
reviewed
Nov 13, 2022
RageKnify
reviewed
Nov 13, 2022
boa_gc/src/cell.rs
Outdated
Comment on lines
537
to
541
| #[allow(clippy::use_self)] | ||
| fn cmp(&self, other: &GcCell<T>) -> Ordering { | ||
| (*self.borrow()).cmp(&*other.borrow()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Any reason against this?
Suggested change
| #[allow(clippy::use_self)] | |
| fn cmp(&self, other: &GcCell<T>) -> Ordering { | |
| (*self.borrow()).cmp(&*other.borrow()) | |
| } | |
| } | |
| fn cmp(&self, other: &Self) -> Ordering { | |
| (*self.borrow()).cmp(&*other.borrow()) | |
| } | |
| } |
There was a problem hiding this comment.
I just thought it might lack a little bit of clarity to be comparing &self vs &Self
There was a problem hiding this comment.
Looking at the documentation for Ord this is the normal signature, I think that is why clippy recommends this.
RageKnify
reviewed
Nov 13, 2022
RageKnify
reviewed
Nov 13, 2022
RageKnify
approved these changes
Nov 13, 2022
Razican
reviewed
Nov 14, 2022
Comment on lines
+20
to
+22
| const ROOT: usize = 1; | ||
| const WRITING: usize = !1; | ||
| const UNUSED: usize = 0; |
There was a problem hiding this comment.
What about using bitflags!{} for this? it could add some useful methods.
boa_gc/src/cell.rs
Outdated
| const UNUSED: usize = 0; | ||
|
|
||
| /// The base borrowflag init is rooted, and has no outstanding borrows. | ||
| pub(crate) const BORROWFLAG_INIT: BorrowFlag = BorrowFlag(1); |
There was a problem hiding this comment.
I guess this is the correct constant, right? We should use it here to avoid issues if we change values, and for clarity:
Suggested change
| pub(crate) const BORROWFLAG_INIT: BorrowFlag = BorrowFlag(1); | |
| pub(crate) const BORROWFLAG_INIT: BorrowFlag = BorrowFlag(ROOT); |
| /// The base borrowflag init is rooted, and has no outstanding borrows. | ||
| pub(crate) const BORROWFLAG_INIT: BorrowFlag = BorrowFlag(1); | ||
|
|
||
| impl BorrowFlag { |
There was a problem hiding this comment.
I think all functions in this impl would benefit from an #[inline] hint. I would also appreciate some documentation.
HalidOdat
approved these changes
Nov 14, 2022
boa_gc/src/internals/eph_box.rs
Outdated
| } | ||
|
|
||
| /// Calls [`Trace::weak_trace()`][crate::Trace] on value | ||
| /// |
boa_gc/src/internals/eph_box.rs
Outdated
| @@ -0,0 +1,142 @@ | |||
| use crate::trace::Trace; | |||
There was a problem hiding this comment.
I think its better to rename this file eph_box.rs => ephemeron_box.rs, since its clearer and matches the pointers/ephemeron.rs
Razican
approved these changes
Nov 14, 2022
raskad
approved these changes
Nov 14, 2022
|
Gonna merge this then. Again, thank you, @nekevss for all the effort you put into this PR! bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Nov 14, 2022
<!--- Thank you for contributing to Boa! Please fill out the template below, and remove or add any information as you feel necessary. ---> Not sure if anyone else may be working on something more substantial/in-depth, but I thought I'd post this. 😄 The basic rundown is that this is more of an untested (and in some ways naïve) draft than anything else. It builds rather heavily on `rust-gc`, and tries to keep plenty of the core aspects so as to not break anything too much, and also to minimize overarching changes were it to actually be merged at some point. This implementation does add ~~a generational divide (although a little unoptimized) to the heap,~~ a GcAlloc/Collector struct with methods, and an ephemeron implementation that allows for the WeakPair and WeakGc pointers.
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
|
FWIW, rust-gc is happy to accept more upstream PRs so that y'all can continue using it. It might be worth maintaining this as a fork that y'all can tailor that y'all occasionally contribute upstream when you think stuff is ready? I do hope the ephemeron stuff can be eventually upstreamed, i know you have a WIP pr) The main thing is that rust-gc should be tailored in ways that ideally can be still flexibly picked up only if needed, but that shouldn't be too hard. In the long run, I suspect boa will want a GC that doesn't need to reference count. I recommend checking out https://github.com/asajeffrey/josephine/ |
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.
Not sure if anyone else may be working on something more substantial/in-depth, but I thought I'd post this. 😄
The basic rundown is that this is more of an untested (and in some ways naïve) draft than anything else. It builds rather heavily on
rust-gc, and tries to keep plenty of the core aspects so as to not break anything too much, and also to minimize overarching changes were it to actually be merged at some point.This implementation does add
a generational divide (although a little unoptimized) to the heap,a GcAlloc/Collector struct with methods, and an ephemeron implementation that allows for the WeakPair and WeakGc pointers.