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

Future plans for this crate #2

Closed
abusch opened this issue Feb 28, 2018 · 7 comments
Closed

Future plans for this crate #2

abusch opened this issue Feb 28, 2018 · 7 comments

Comments

@abusch
Copy link
Contributor

abusch commented Feb 28, 2018

Hi,

it seems that the placement_in and related features are scheduled to be removed soon, as they have some unresolved issues and have been in limbo for a while. See rust-lang/rust#48333 (they might come back one day under a different form...).

How necessary are these features for light_arena to work? Unfortunately crates.io doesn't offer many alternatives :(

@Twinklebear
Copy link
Owner

Twinklebear commented Feb 28, 2018

They're pretty necessary for it to work truly as a memory arena, without the placement new/placement in functionality we'd have to construct objects on the stack and copy them over into the arena. It's maybe not so bad for some use cases, but adds some cost which we don't incur right now. Other uses like allocating something larger than can be put on the stack in the arena would no longer work.

Hopefully the feature will make its way back into Rust soon, this is pretty unfortunate to see go. The feature has been pretty stagnant lately though.

@abusch
Copy link
Contributor Author

abusch commented Mar 1, 2018

I looked into this a bit, as I was wondering how something like Box::new<Foo {...}> works. It turns out that in most cases, the compiler is able to optimize away the stack allocation + copy/move, at least in release mode. However, it also looks like there are some bugs in some cases: rust-lang/rust#41160 :(

I also had a look at the rust compiler. I knew it had a TypedArena<T> already, but it looks like it now also has a DroplessArena which I might try to extract and reuse. Although it would probably suffer from the problems you mention? I guess having a stack alloc + copy sometimes is still better than no arena at all presumably...

@Twinklebear
Copy link
Owner

Interesting about Box::new, part of what the Arena will avoid though is the small heap allocations produced by all the Box::new calls.

DroplessArena might be a good fit as a temporary replacement until the placement new stuff gets figured out in the language. A stack alloc and copy would not be as bad as having the large number of small box allocations. For a temporary workaround (I hope) it wouldn't be so bad.

@abusch
Copy link
Contributor Author

abusch commented Mar 7, 2018

I quickly hacked up light_arena to remove the use of placement_in_syntax and use a simple alloc() method instead. See https://github.com/Twinklebear/light_arena/compare/master...abusch:remove_placement_syntax?expand=1

I managed to get the tests to pass, except the ones that try to allocate an array bigger than 2MB, which shows that the stack allocation isn't optimized away :( I guess Box::new is treated in a special way (looking at it, it is still implemented using the box syntax).

I've adapted my raytracer to use the modified version, and it seems to work fine. I haven't noticed any performance impact so far, but I've only tested with a very simple scene... I guess this will have to do until a better solution comes up.

@Twinklebear
Copy link
Owner

Looks good! For allocating the buffers in my renderer I can switch them to using some Vec storage which is re-used, the BRDFs and such should be ok to do a stack alloc and a copy, since they're kind of small. From what I remember Box still gets some special treatment by the compiler, so we likely won't be able to do things beyond the stack size limit until placement new comes back.

I wonder if this is best released as a separate temporary crate and we can mark light arena as "temporarily broken" while placement new gets worked out. We could maintain the code in a branch in this repo, but release it under some other name I guess. I'll find some time to test out tray_rust as well with your changes.

@abusch
Copy link
Contributor Author

abusch commented Mar 10, 2018

Any of these options if fine by me. The one thing I haven't done is modify the documentation and the doc tests because I was lazy :) Let me know how your test goes!

@Twinklebear
Copy link
Owner

Hey @abusch , sorry for the (extreme) delay, I got back to this and merged in your PR to have the crate act for now as more of memory pool. Then we'll see where the rust placement new stuff ends up and I can update it later when the feature stabilizes.

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

No branches or pull requests

2 participants