feat/chore: introduce fallible alternatives for MutableBuffer#10317
feat/chore: introduce fallible alternatives for MutableBuffer#10317Rich-T-kid wants to merge 9 commits into
Conversation
once this is merged Ill work on updating call sites. From my understanding this isn't always a 1-1 switch, its only possible to use the fallible alternative if the calling functions also returns an error |
|
I just saw your PR when I raised my own one: #10329! Sorry for the double up of work... |
@cetra3 No worries — to avoid having the maintainers review two separate PRs, would you mind closing #10329 and reviewing this one instead? That way we can consolidate our approaches and make it easier to review. We can also split off the follow-up work of propagating the fallible version of the API throughout the arrow codebase. Let me know if this makes sense! |
|
Hey no worries I will close it off. I do like the following differences from this PR (if you could fold them in to this one):
|
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
It doesn't duplicate the logic, i.e, the fallible versions are just try_().unwrap() which means we don't need to adjust it in two places
this would be pretty neat to have if possible
This makes sense, this is a much cleaner pattern and it makes migration later down the line.
I can see how this may be a worry but I think the number of ways these functions can fail is very small. I think an enum is more applicable in this case. happy to hear other thoughts on this. |
349c425 to
56c9f81
Compare
56c9f81 to
ad30053
Compare
|
@cetra3 could you take another look? |
| Ok(()) | ||
| } | ||
| #[cold] | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
setting this to #[expect(dead_code)] work fine for local development but it cause this ci to fail.
| @@ -140,13 +174,13 @@ impl MutableBuffer { | |||
| NonNull::new(raw_ptr).unwrap_or_else(|| handle_alloc_error(layout)) | |||
There was a problem hiding this comment.
This is still a panic on allocation failure
There was a problem hiding this comment.
good catch. The one way this could fail is if we OOM.
We could do something like this
| NonNull::new(raw_ptr).unwrap_or_else(|| handle_alloc_error(layout)) | |
| let raw_ptr = unsafe { std::alloc::alloc(layout) }; | |
| if raw_ptr.is_null() { | |
| return Err(MutableBufferError::OOM); | |
| } | |
| NonNull::new(raw_ptr).unwrap_or_else(|| handle_alloc_error(layout)) |
But I think handle_alloc_error already deals with this. I think running out of memory is a condition where the program should panic.
We could bubble up the error to the caller but there aren't many things you can do when your process runs out of memory, I'm sure it would be similar handling to what handle_alloc_error already does. I don't have too strong of an opinion either way, happy to try either approach.
There was a problem hiding this comment.
I use a memory limited global allocator in some scenarios already, and if we want to move to custom allocators, we don't want to capture the panic here.
There was a problem hiding this comment.
I.e, in my PR I had a simple match statement:
match NonNull::new(raw_ptr) {
Some(data) => data,
None => return Err(TryReserveError::new(layout.size())),
}There was a problem hiding this comment.
make sense, 2d0f74fe takes a similar approach.
I use a memory limited global allocator in some scenarios already
Im curious as to why? is this mostly for testing in low memory enviroments?
There was a problem hiding this comment.
My interest in this PR is to help with: apache/datafusion#22758
The linux allocator famously does not limit allocations, and so you can get reaped by the OOM killer easily.
If we have a global allocator that internally limits how many allocations it dishes out, we can course correct in code if an error is returned. A panic still works, but this is a panic which is a pretty blunt instrument and means we can't take corrective action in code easily, which is what I found in testing.
I.e, if we have a memory limited allocator we can use this as a gauge to whether we need to spill memory to disk etc... so it's very useful as a tool there
There was a problem hiding this comment.
nice, I also have interest in apache/datafusion#22758 & #8938
If we have a global allocator that internally limits how many allocations it dishes out, we can course correct in code if an error is returned
make sense.
Would the next step be to replace all OOM panics with error propagation? Off the top of my head, I think arrow-buffer's crate deals with memory management most directly out of all the crates, so it would be a good place to start.
There was a problem hiding this comment.
are there any downstream concerns with replacing what we previously did (calling handle_alloc_error()) with just an error and relying on downstream to properly handle (in this case theyd just do a regular panic now)
2d0f74e to
0b1a641
Compare
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
it would be good to preserve some of the useful comments as it looks like theyre getting lost in the port
and i wonder if we'll need to run benches for these changes (if i can find appropriate ones)
| @@ -140,13 +174,13 @@ impl MutableBuffer { | |||
| NonNull::new(raw_ptr).unwrap_or_else(|| handle_alloc_error(layout)) | |||
There was a problem hiding this comment.
are there any downstream concerns with replacing what we previously did (calling handle_alloc_error()) with just an error and relying on downstream to properly handle (in this case theyd just do a regular panic now)
6f03bec re-introduces the comments
im not sure. I think the best person to ask would be @alamb. from the docs for handle_alloc_error
it seems that once this branch is hit there isn't much for the process to do anyway? in both cases it aborts the process. |
|
run benchmarks mutable_buffer_repeat_slice buffer_create builder |
yeah I was mainly looking at this part of its doc:
but its probably fine i guess? |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
4 participants
Which issue does this PR close?
Rationale for this change
see #9843.
MutableBufferis a low-level building block used throughout the Arrow implementation. Every growth operation,reserve,resize,push,extend_from_slice, etc, can panic on arithmetic overflow or an invalid allocation layout. In library code that needs to surface errors gracefully. This PR adds methods to return errors instead of panicking.I made new methods instead of adding conditional checks/wrappers for each method because its easier to slowly roll these changes out to call sites instead of swapping.
What changes are included in this PR?
Are these changes tested?
Currently these methods aren't wired up to anything in the code base so I didn't include any test. Id be happy to include some. The implementations of the fallible API's are 1-1 to their non-fallible counter parts so i'm not sure if this would be needed.
Are there any user-facing changes?
yes, users can now work with errors instead of having their programs panic.