Optimize representation of Error without backtrace #20
Conversation
cramertj
force-pushed
the
opt-nobacktrace
branch
from
6f32767
to
16f2756
Compare
This change modifies Error to not store empty backtraces. This saves space and allows zero-sized errors without backtraces to be stored without any allocation.
cramertj
force-pushed
the
opt-nobacktrace
branch
from
16f2756
to
fec787c
Compare
| // Attempt to add a backtrace | ||
| let backtrace = Backtrace::new(); | ||
| if backtrace.is_none() { | ||
| Box::new(failure) as Box<FailOrWithBacktrace> |
There was a problem hiding this comment.
Is this actually guaranteed not to allocate if the failure is a ZST?
There was a problem hiding this comment.
Yes, but to be clear, this code doesn't rely on that behavior for correctness.
| Ok(ret) | ||
| } | ||
| _ => Err(self) | ||
| if self.downcast_ref::<T>().is_some() { |
There was a problem hiding this comment.
I would like more comments on how this rewrite of the unsafe code works and is correct. To me it looks like this is wrong, but I'm not confident I'm right about that.
In particular, if there is a backtrace, you don't seem to ever actually drop the backtrace here. This is what the ptr::read stuff was about.
There was a problem hiding this comment.
Sure, I'll add a comment. The drop is occurring implicitly since Box::from_raw(ptr as *mut WithBacktrace<T>) creates a Box<WithBacktrace<T>>. The failure field is moved out, and the rest of the box (including the remaining backtrace field) is dropped.
I don't think saving a couple words in the heap is a win worth the additional complexity, so this commit is only worth it because (if) it allows ZST errors to not allocate when backtraces are turned off. I'm not certain that this will actually work, though. Do we guarantee that a Box will have a null data pointer if the data would be a ZST? Another concern we've talked about is how this will interact with #9. If we start double boxing things, we don't go from this being an alloc to no allocs, we go from 2 allocs to 1 (which is much less important). All in all I'm increasingly unsure about what direction we should be optimizing in here. As @cramertj and I have talked about over IRC, there is an 'optimal' (highly unsafe) representation in which you store the vtable inline with the data, making it one word, and use alignment to mask a vtable vs heap pointer so you also don't alloc in the ZST/no-backtrace case. But that's a lot of complexity. Finally, there's a question fo guarantees that both this and #9 raise. RIght now, I'd feel comfortable guaranteeing that I know I told you a few days ago that I thought separating this from the other PR would be an easier merge, sorry to backpedal on that 😅. |
|
No worries! There's a lot of things to consider here-- I agree that we wouldn't want to pretend to offer guarantees that we don't actually provide.
Yup, totally agree.
No, we do not. In fact, we guarantee the opposite-- the data pointer cannot be a null pointer (at the moment, it's usually 0x1, but that's probably going to change soon). However, we do guarantee that no allocation is actually performed.
Yes, if we go that route I'd advocate for either (a) removing the optimization or (b) using the leftover-from-alignment bits of the top-level
As far as future size/optimization guarantees, I'd assumed that you wouldn't provide any concrete performance guarantees until the library had more time to mature and we'd gotten some practical experience/benchmarks. If you want to provide an explicit guarantee that |
|
I think I'm inclined to do neither this nor #9 prior to the 0.1 release, and try to collect feedback based on use about which direction the performance issues tilt. |
|
That's fine w/ me-- these are all backwards compatible. |
This change modifies Error to not store empty backtraces.
This saves space and allows zero-sized errors without
backtraces to be stored without any allocation.