rust-gamedev / wg

Coordination repository of the Game Development Working Group
514 stars 10 forks source link

Meeting: Nov 27, 2019 #70

Closed Wodann closed 4 years ago

Wodann commented 5 years ago

Action items:

Lokathor commented 5 years ago

action item: minimal frame allocator with bump allocation and a reset function.

Lokathor commented 5 years ago

possiblity: adding "crate assumptions" metadata in crates.io or on the ecosystem guide. Eg: "uses thread local storage", "uses threads", etc

Lokathor commented 5 years ago

action item assignment: I will ask around about thread pool unification.

Lokathor commented 5 years ago

Started an issue for the minimal bump alloc: https://github.com/rust-gamedev/wg/issues/71

Lokathor commented 5 years ago

"global thread pools": withoutboats posted this, which seems very related, https://twitter.com/withoutboats/status/1195030095800471552

Lokathor commented 5 years ago

Meeting is tomorrow

Reminder

The meeting is scheduled for 18:00 UTC

Which, because of daylight savings changes, is now 1 hour different from the time we were meeting in the summer!

Updated times for some time zones:

and so on!

Lokathor commented 5 years ago

(We can discuss if we should make the meeting times follow daylight savings and such, but currently the readme's stance is clear that we don't follow daylight savings)

Wodann commented 5 years ago

I implemented a bump allocator using the alloc-wg's traits.

To achieve that I proposed some changes in this PR.

Lokathor commented 5 years ago

aclysma wanted https://github.com/rust-gamedev/wg/pull/44 to get some talk, it's been sitting a while

will probably have to get updated some before we can merge it.

Lokathor commented 5 years ago

It was just me and wodann this time.

Notes:

95th commented 5 years ago

@Wodann You may be interested in my fork of bumpalo using alloc-wg crate. I didn't need alloc-wg's traits to take immutable self.

Wodann commented 5 years ago

@95th Nice! I had another look to see why we might have a discrepancy, but my version of the bump allocator is a generic that accepts any allocator that implements the BuildAllocRef, DeallocRef and AllocRef. Bumpalo just uses a static alloc function.

When using a generic, the AllocRef::alloc(&mut self, ...) function would force Bumpalo's Bump::alloc(...) to be a mutable reference as well. I think that is where it breaks down.

95th commented 4 years ago

That static fn remained from original implementation. I have implemented alloc wg traits here so you can pass immutable reference &Bump in alloc generic collections.

For example: https://github.com/95th/bumpalo/blob/15aa51d51f730e7a271cc16917671dd6fdcb445c/tests/string.rs#L8

TimDiekmann commented 4 years ago

Is it possible for someone to summarize the meeting in terms of the alloc-wg crate? If I understood that correctly, &mut self seems to have posed a problem, but as @95th pointed out, it still seems possible to implement a bump allocator via a mutable reference (note, that the current Alloc trait also takes a mutable reference).

But I would also be interested to know to what extent the implemented proposals were accepted:

cc @glandium @Ericson2314 @gnzlbg

SimonSapin commented 4 years ago

Does the associated error-type have more disadvantages than advantages? (cc @SimonSapin, I know you have a pretty strong opinion regarding this)

I’m skeptical the an associated error type is all that useful as a way to return additional diagnostic information. At least I haven’t seen concrete use cases, only what feels like attempts at being maximally generic out of principle. But this isn’t a strong opinion.

I am opposed to the idea that every API that could ever involve allocation should always return a Result, and that infallible APIs would "naturally" fall out of setting the associated error type to !. This is incompatible with everything that already exists (for example Vec::push is stable and doesn’t return a Result), and would be extremely ergonomic. Result<Foo, !> is not the same as Foo to callers, even if we add an unwrap-like method guaranteed not to panic.

Even without going that far, planning to set an allocator’s associated error type to ! seems to me that it’s handling errors at the wrong layer.

Finally, #[global_allocator] under the hood passes pointers to extern functions with a fixed API, so anything type-level doesn’t work there.


Having said that, what do you think are advantages or uses of an associated error type?

Lokathor commented 4 years ago

(I thought I had recorded the meeting, but I'm a dummy and the audio settings were messed up, and so there was no audio.)

So we got this methods here https://github.com/Wodann/alloc-bump-rs/blob/master/src/lib.rs#L64 which moves a T into the allocator's memory region and return the &mut T to it. If you do this, and you have alloc_t accept &mut self, there's no way to allocate two things, because the second thing can't get a &mut self to allocate since the lifetime of the first &mut T is still live. 1) You can make the output &'static mut T but that seems error prone. 2) You can make the input &self, which unblocks this.

Then when you have everything taking &self, there's another problem: when you reset the allocator you can potentially double-assign some memory and then there's UB because you have two &mut T to the same memory span.

I didn't get a hang of the point of BuildAllocRef during the meeting. Is it just a weird way to have a reference that isn't a literal reference?

In terms of associated errors, if the allocator wants to have errors be useful, just make an extension trait for it that adds a "get last error" method or something. Like an allocator should be returning *mut u8 or *mut c_void, perhaps Option<NonNull<u8>>, or Option<NonNull<c_void>> if you wanna get fancy. But I haven't followed the allog-wg discussions at all so maybe there's some super good reason to not keep it simple, but keeping it simple is what I'd favor, if possible.

Wodann commented 4 years ago

That static fn remained from original implementation. I have implemented alloc wg traits here so you can pass immutable reference &Bump in alloc generic collections.

For example: https://github.com/95th/bumpalo/blob/15aa51d51f730e7a271cc16917671dd6fdcb445c/tests/string.rs#L8

@95th I understand that your changes allow the bumpalo Bump allocator to use the alloc-wg's String, Box, etc. types. I think you might be missing my point, though.

At the moment, the bumpalo Bump allocator uses the static alloc function to allocate its chunks. All of the impl Bump functions still use that alloc function. In my implementation, I use the AllocRef trait to allow users to specify their own allocator to use for chunk allocation. Once you take that approach, you cannot have Bump::alloc or Bump::alloc_with accept a shared reference, as the AllocRef::alloc function that it tries to call requires a mutable reference.

You can try changing your Bump struct into a generic: struct Bump<A: DeallocRef = Global> that stores the A::BuildAlloc for usage instead of the static alloc function. I think you'll run into the same issue as me then.

Wodann commented 4 years ago

@TimDiekmann I hope this answers your questions.

how natural does the BuildAllocRef feel like? This is by far the biggest change in comparison to the Alloc trait.

I implemented the BuildAllocRef trait for a shared reference, so I was merely returning self. I am not sure if it has any benefits and in what case it would have benefits. E.g. now I am storing a generic that implements the BuildAllocRef trait inside my structure, but I don't see any advantage over just storing an AllocRef.

I also had to look at the Global allocator implementation to figure out what it was there for and how to interconnect the different traits. As such, I'd say that it wasn't intuitive.

Does the associated error-type have more disadvantages than advantages? (cc @SimonSapin, I know you have a pretty strong opinion regarding this)

I didn't actually return any error information, but one might. I am impartial to having an Option or Result type. Do you have an idea what the performance overhead might be?

Is there anything against NonZeroLayout?

No complaints. I appreciate the extra guarantee.

Ericson2314 commented 4 years ago

I think you can do something like this:

impl<ChunkAlloc> AllocRef for Bump<ChunkAlloc> where ChunkAlloc: AllocRef {}
impl<ChunkAlloc> AllocRef for &Bump<&ChunkAlloc> where &ChunkAlloc: AllocRef {}

Now you support concurrent access if and only if the underlying chunk allocator supports it. Perfect!

Lokathor commented 4 years ago

@TimDiekmann actually now that i think about it, I'd personally like if NonZeroLayout had exactly one NonZero value, because it makes the Option<NonZeroLayout> clearly have 0 for None and a real value for non-zero. With two internal NonZero values it doesn't work out like that and ends up not playing nice with zeroed memory.

TimDiekmann commented 4 years ago

Does the associated error-type have more disadvantages than advantages?

I didn't actually return any error information, but one might. I am impartial to having an Option or Result type. Do you have an idea what the performance overhead might be?

I haven't done any performance checks, but as long as the underlying error type is a ZST, this should have no impact at all.

Lokathor commented 4 years ago

Closing this up so we don't have too many old meetings issues.

If you have more to say please feel free to open a new issue for it (if necessary) or just move discussion to a newer meeting issue.