Closed Tamschi closed 3 years ago
Ah whoops, Debug
isn't available on huge arrays in 1.44. Going to fix that in a moment.
I'm confident I'll use this now (and certain that the alternative would be either much slower or really messy), so I'm converting this draft into a proper PR.
Thanks for opening this PR.
Is there a reason that you can't move the fallible parts out of the closure, before the call to try_alloc_with
? Can you share your example code?
Sure! For reference, this is the actual commit in my project. Apologies in advance for the amount of macro templates¹.
The gist is that I'm working on a web frontend framework/templating DSL where the render methods use bumpalo to emit a VDOM graph every frame.² Individual Node
s are small, so for the most part these are passed around and returned by value.
When it comes to child nodes, I do have to allocate though, so I write the children to the heap like this (de-qualified for brevity):
Node::Multi
(VDOM-only grouping) 🔖:
Node::Multi(&*#bump.try_alloc_with(
|| -> Result::<_, GUIError> { Ok([
#(#m,)*
])}
)?)
for HTML elements 🔖:
let children = quote_spanned! {child_stream.span()=>
&*#bump.try_alloc_with(
|| -> Result<_, GUIError> {
Ok([#child_stream])
}
)?
};
Depending on what the individual app does, these arrays could get fairly large, and this would in most cases be hot code, so I'd like to put them onto the heap immediately. The individual items can really be anything, including verbatim Rust blocks and/or expressions, so I have no way of knowing if they're fallible or not outside of very plain HTML expressions.
It's possible I could generate let
bindings for each element and do the error escalation there, then move everything into the closure (even if that feels icky. I was going for 'as if handwritten' for the generated code, partially to get better compiler errors on type mismatch etc.. Probably not an issue here though).
However, since any error would prevent the allocation in that case, I'm not confident the compiler could reorder that.
These particular GUIError
s here are essentially panic-equivalent, so once I have the necessary infrastructure otherwise, actually hitting one of them will be very rare and, with the default handlers, eject part of the GUI from further updates. The only reason I haven't made them plain panics is that Rust on Wasm can't unwind yet, but a DI error shouldn't always crash the entire application.
¹ and please disregard where I use try_alloc_with
for single items, that's fixed now.
² This was actually partially inspired by Dodrio, but I needed a bit more meta data, so I ended up writing it from scratch. It's not very optimised yet, though, so for now the DOM renderer is 100% Rust.
Thanks for the context.
As you noted, the try_
prefix is already used for fallible allocation; we want something different for fallible initialization. Initialization uses the _with
suffix and I think we could do a _try_with
suffix for fallible initialization, which is maybe a little clunky but gives us this logical and backwards-compatible table of methods and their fallibility:
fallible allocation | infallible allocation | |
---|---|---|
fallible init | try_alloc_try_with |
alloc_try_with |
infallible init | try_alloc_with |
alloc_with |
The try_alloc_try_with
variant will need a custom error enum that is either an allocation error or the user-supplied closure's error. The _try_with
variants will also need to free the allocation if initialization fails, rather than leak it.
I would be willing to merge a PR that implements this scheme. I don't think it should be hard to evolve this PR into that.
Sounds good to me. I'm not sure when, exactly, but I should be able to add the extra methods, tests and benches later this week.
I believe this completes the PR, unless I made a copy-paste typo somewhere in the documentation and didn't find it yet. Thanks for considering this!
As an aside, I tried to unify some of the code (see (try_) unification.patch.txt), but this made some code paths significantly slower: cargo bench
log
Note: I'm not sure these measurements are any good, but it's the best I could do on my tablet running only a CMD window and the task manager.
Finished bench [optimized] target(s) in 14.34s
Running target\release\deps\benches-9e8bb6bc6844d2ce.exe
Gnuplot not found, disabling plotting
alloc/small time: [30.787 us 31.114 us 31.545 us]
thrpt: [317.01 Melem/s 321.40 Melem/s 324.81 Melem/s]
change:
time: [-3.6026% -1.1280% +1.3719%] (p = 0.39 > 0.05)
thrpt: [-1.3534% +1.1408% +3.7372%]
No change in performance detected.
Found 11 outliers among 100 measurements (11.00%)
4 (4.00%) high mild
7 (7.00%) high severe
Benchmarking alloc/big: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 15.4s or reduce sample count to 30
alloc/big time: [3.0308 ms 3.0357 ms 3.0408 ms]
thrpt: [3.2886 Melem/s 3.2941 Melem/s 3.2995 Melem/s]
change:
time: [-0.7714% -0.4196% -0.1099%] (p = 0.02 < 0.05)
thrpt: [+0.1100% +0.4214% +0.7774%]
Change within noise threshold.
Found 6 outliers among 100 measurements (6.00%)
3 (3.00%) low mild
2 (2.00%) high mild
1 (1.00%) high severe
alloc-with/small time: [40.164 us 41.466 us 42.741 us]
thrpt: [233.97 Melem/s 241.16 Melem/s 248.98 Melem/s]
change:
time: [+4.7684% +8.4004% +12.367%] (p = 0.00 < 0.05)
thrpt: [-11.006% -7.7494% -4.5514%]
Performance has regressed.
Benchmarking alloc-with/big: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 15.3s or reduce sample count to 40
alloc-with/big time: [3.0190 ms 3.0240 ms 3.0292 ms]
thrpt: [3.3012 Melem/s 3.3069 Melem/s 3.3124 Melem/s]
change:
time: [-0.1205% +0.2303% +0.5757%] (p = 0.20 > 0.05)
thrpt: [-0.5724% -0.2298% +0.1206%]
No change in performance detected.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) low severe
2 (2.00%) low mild
1 (1.00%) high mild
alloc-try-with/small, small
time: [76.797 us 77.065 us 77.355 us]
thrpt: [129.27 Melem/s 129.76 Melem/s 130.21 Melem/s]
change:
time: [+1.5634% +2.0473% +2.5131%] (p = 0.00 < 0.05)
thrpt: [-2.4515% -2.0063% -1.5393%]
Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
2 (2.00%) high mild
2 (2.00%) high severe
Benchmarking alloc-try-with/small, big: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 35.2s or reduce sample count to 20
alloc-try-with/small, big
time: [6.9628 ms 6.9709 ms 6.9792 ms]
thrpt: [1.4328 Melem/s 1.4345 Melem/s 1.4362 Melem/s]
change:
time: [-2.6759% -2.3330% -1.9903%] (p = 0.00 < 0.05)
thrpt: [+2.0307% +2.3887% +2.7495%]
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
1 (1.00%) low mild
2 (2.00%) high mild
2 (2.00%) high severe
Benchmarking alloc-try-with/big, small: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 32.8s or reduce sample count to 20
alloc-try-with/big, small
time: [6.4648 ms 6.4749 ms 6.4852 ms]
thrpt: [1.5420 Melem/s 1.5444 Melem/s 1.5468 Melem/s]
change:
time: [-3.0961% -2.6848% -2.2692%] (p = 0.00 < 0.05)
thrpt: [+2.3219% +2.7589% +3.1951%]
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
3 (3.00%) high mild
1 (1.00%) high severe
Benchmarking alloc-try-with/big, big: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 23.6s or reduce sample count to 30
alloc-try-with/big, big time: [4.6599 ms 4.6679 ms 4.6764 ms]
thrpt: [2.1384 Melem/s 2.1423 Melem/s 2.1460 Melem/s]
change:
time: [-0.2901% +0.0719% +0.4742%] (p = 0.74 > 0.05)
thrpt: [-0.4719% -0.0719% +0.2909%]
No change in performance detected.
Found 6 outliers among 100 measurements (6.00%)
1 (1.00%) low mild
2 (2.00%) high mild
3 (3.00%) high severe
alloc-try-with-err/small, small
time: [114.27 us 114.54 us 114.82 us]
thrpt: [87.091 Melem/s 87.306 Melem/s 87.509 Melem/s]
change:
time: [-3.7575% -3.2652% -2.7494%] (p = 0.00 < 0.05)
thrpt: [+2.8271% +3.3754% +3.9042%]
Performance has improved.
Found 11 outliers among 100 measurements (11.00%)
8 (8.00%) high mild
3 (3.00%) high severe
Benchmarking alloc-try-with-err/small, big: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 23.1s or reduce sample count to 30
alloc-try-with-err/small, big
time: [4.5740 ms 4.5782 ms 4.5828 ms]
thrpt: [2.1821 Melem/s 2.1843 Melem/s 2.1863 Melem/s]
change:
time: [+7.2851% +7.5903% +7.8967%] (p = 0.00 < 0.05)
thrpt: [-7.3187% -7.0548% -6.7904%]
Performance has regressed.
Found 9 outliers among 100 measurements (9.00%)
4 (4.00%) high mild
5 (5.00%) high severe
Benchmarking alloc-try-with-err/big, small: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 16.5s or reduce sample count to 30
alloc-try-with-err/big, small
time: [3.2458 ms 3.2525 ms 3.2612 ms]
thrpt: [3.0664 Melem/s 3.0745 Melem/s 3.0809 Melem/s]
change:
time: [-0.9435% -0.3083% +0.2414%] (p = 0.34 > 0.05)
thrpt: [-0.2409% +0.3093% +0.9525%]
No change in performance detected.
Found 9 outliers among 100 measurements (9.00%)
6 (6.00%) high mild
3 (3.00%) high severe
Benchmarking alloc-try-with-err/big, big: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 9.8s or reduce sample count to 50
alloc-try-with-err/big, big
time: [1.9423 ms 1.9464 ms 1.9512 ms]
thrpt: [5.1250 Melem/s 5.1376 Melem/s 5.1485 Melem/s]
change:
time: [+7.2287% +9.9075% +12.335%] (p = 0.00 < 0.05)
thrpt: [-10.981% -9.0144% -6.7414%]
Performance has regressed.
try-alloc/small time: [36.296 us 36.894 us 37.618 us]
thrpt: [265.83 Melem/s 271.05 Melem/s 275.52 Melem/s]
change:
time: [+1.4426% +4.7548% +8.1826%] (p = 0.00 < 0.05)
thrpt: [-7.5637% -4.5389% -1.4221%]
Performance has regressed.
Found 17 outliers among 100 measurements (17.00%)
2 (2.00%) high mild
15 (15.00%) high severe
Benchmarking try-alloc/big: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 15.5s or reduce sample count to 30
try-alloc/big time: [3.0517 ms 3.0568 ms 3.0619 ms]
thrpt: [3.2660 Melem/s 3.2714 Melem/s 3.2768 Melem/s]
change:
time: [-0.1380% +0.3655% +0.8382%] (p = 0.14 > 0.05)
thrpt: [-0.8313% -0.3642% +0.1382%]
No change in performance detected.
Found 5 outliers among 100 measurements (5.00%)
1 (1.00%) low mild
3 (3.00%) high mild
1 (1.00%) high severe
try-alloc-with/small time: [37.099 us 38.511 us 39.941 us]
thrpt: [250.37 Melem/s 259.66 Melem/s 269.55 Melem/s]
change:
time: [-6.8818% -3.7387% -0.3998%] (p = 0.03 < 0.05)
thrpt: [+0.4014% +3.8839% +7.3904%]
Change within noise threshold.
Found 20 outliers among 100 measurements (20.00%)
20 (20.00%) high mild
Benchmarking try-alloc-with/big: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 15.2s or reduce sample count to 40
try-alloc-with/big time: [3.0156 ms 3.0213 ms 3.0267 ms]
thrpt: [3.3039 Melem/s 3.3098 Melem/s 3.3161 Melem/s]
change:
time: [+0.0297% +0.3817% +0.7227%] (p = 0.04 < 0.05)
thrpt: [-0.7175% -0.3802% -0.0297%]
Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) low mild
1 (1.00%) high severe
try-alloc-try-with/small, small
time: [84.321 us 84.470 us 84.637 us]
thrpt: [118.15 Melem/s 118.39 Melem/s 118.59 Melem/s]
change:
time: [+5.1242% +5.5777% +6.0258%] (p = 0.00 < 0.05)
thrpt: [-5.6833% -5.2830% -4.8744%]
Performance has regressed.
Found 9 outliers among 100 measurements (9.00%)
4 (4.00%) high mild
5 (5.00%) high severe
Benchmarking try-alloc-try-with/small, big: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 35.1s or reduce sample count to 20
try-alloc-try-with/small, big
time: [6.9179 ms 6.9297 ms 6.9429 ms]
thrpt: [1.4403 Melem/s 1.4431 Melem/s 1.4455 Melem/s]
change:
time: [-3.0434% -2.6458% -2.3006%] (p = 0.00 < 0.05)
thrpt: [+2.3547% +2.7177% +3.1389%]
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high mild
Benchmarking try-alloc-try-with/big, small: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 32.7s or reduce sample count to 20
try-alloc-try-with/big, small
time: [6.4627 ms 6.4693 ms 6.4760 ms]
thrpt: [1.5442 Melem/s 1.5458 Melem/s 1.5473 Melem/s]
change:
time: [-3.2540% -2.7620% -2.3663%] (p = 0.00 < 0.05)
thrpt: [+2.4236% +2.8404% +3.3635%]
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
5 (5.00%) high mild
1 (1.00%) high severe
Benchmarking try-alloc-try-with/big, big: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 23.5s or reduce sample count to 30
try-alloc-try-with/big, big
time: [4.6491 ms 4.6557 ms 4.6623 ms]
thrpt: [2.1449 Melem/s 2.1479 Melem/s 2.1510 Melem/s]
change:
time: [+1.0471% +1.3128% +1.5711%] (p = 0.00 < 0.05)
thrpt: [-1.5468% -1.2958% -1.0363%]
Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) low mild
3 (3.00%) high mild
try-alloc-try-with-err/small, small
time: [119.31 us 119.58 us 119.88 us]
thrpt: [83.413 Melem/s 83.625 Melem/s 83.817 Melem/s]
change:
time: [-0.5209% -0.1346% +0.2731%] (p = 0.52 > 0.05)
thrpt: [-0.2724% +0.1347% +0.5237%]
No change in performance detected.
Found 13 outliers among 100 measurements (13.00%)
5 (5.00%) high mild
8 (8.00%) high severe
Benchmarking try-alloc-try-with-err/small, big: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 22.5s or reduce sample count to 30
try-alloc-try-with-err/small, big
time: [4.4370 ms 4.4419 ms 4.4469 ms]
thrpt: [2.2487 Melem/s 2.2513 Melem/s 2.2538 Melem/s]
change:
time: [-0.2756% -0.0040% +0.2875%] (p = 0.98 > 0.05)
thrpt: [-0.2867% +0.0040% +0.2763%]
No change in performance detected.
Found 4 outliers among 100 measurements (4.00%)
4 (4.00%) high severe
Benchmarking try-alloc-try-with-err/big, small: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 16.4s or reduce sample count to 30
try-alloc-try-with-err/big, small
time: [3.2361 ms 3.2406 ms 3.2455 ms]
thrpt: [3.0811 Melem/s 3.0859 Melem/s 3.0902 Melem/s]
change:
time: [-0.3445% -0.0320% +0.2655%] (p = 0.84 > 0.05)
thrpt: [-0.2648% +0.0320% +0.3457%]
No change in performance detected.
Found 8 outliers among 100 measurements (8.00%)
5 (5.00%) high mild
3 (3.00%) high severe
Benchmarking try-alloc-try-with-err/big, big: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 9.2s or reduce sample count to 50
try-alloc-try-with-err/big, big
time: [1.8183 ms 1.8210 ms 1.8238 ms]
thrpt: [5.4830 Melem/s 5.4916 Melem/s 5.4997 Melem/s]
change:
time: [-0.5362% -0.1418% +0.2820%] (p = 0.50 > 0.05)
thrpt: [-0.2812% +0.1420% +0.5391%]
No change in performance detected.
Found 11 outliers among 100 measurements (11.00%)
6 (6.00%) high mild
5 (5.00%) high severe
format-realloc/format-realloc/10
time: [113.18 ns 113.70 ns 114.21 ns]
thrpt: [87.558 Melem/s 87.950 Melem/s 88.357 Melem/s]
change:
time: [-0.8027% -0.1835% +0.4548%] (p = 0.57 > 0.05)
thrpt: [-0.4527% +0.1838% +0.8092%]
No change in performance detected.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mild
format-realloc/format-realloc/80
time: [113.68 ns 114.24 ns 114.76 ns]
thrpt: [697.12 Melem/s 700.30 Melem/s 703.70 Melem/s]
change:
time: [-1.4246% -0.6593% +0.1217%] (p = 0.10 > 0.05)
thrpt: [-0.1216% +0.6637% +1.4452%]
No change in performance detected.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severe
format-realloc/format-realloc/270
time: [113.88 ns 114.38 ns 114.88 ns]
thrpt: [2.3503 Gelem/s 2.3606 Gelem/s 2.3710 Gelem/s]
change:
time: [+0.0874% +0.6710% +1.2582%] (p = 0.03 < 0.05)
thrpt: [-1.2425% -0.6665% -0.0874%]
Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mild
format-realloc/format-realloc/640
time: [113.75 ns 114.26 ns 114.72 ns]
thrpt: [5.5787 Gelem/s 5.6014 Gelem/s 5.6262 Gelem/s]
change:
time: [-0.6360% -0.0776% +0.5180%] (p = 0.79 > 0.05)
thrpt: [-0.5153% +0.0776% +0.6400%]
No change in performance detected.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild
Well, almost. The question of alloc::AllocErr
is still open, which also means that AllocOrInitError<_>
isn't Error
yet. I can already implement Display
, though.
@fitzgen I think I've addressed everything now, aside from your concerns about not using <Bump as Alloc>::dealloc
(see my other comment).
I also made a few smaller last-minute changes to bits and pieces I noticed while reading through the full changeset again, like removing the Unpin
bounds. You can find those separately by checking the commits on my squash backup branch.
Looks like there is a valgrind failure in CI that wasn't in the previous iteration of this PR. Mind looking into it?
As soon as that is resolved and CI is green, I'll merge this! Thanks @Tamschi!
I saw that, but it looked like these were in tests that don't access any code this pull request touches. From what I can tell, it's in the panic handling that happens here:
#[test]
#[should_panic(expected = "out of memory")]
fn alloc_slice_overflow() {
let b = Bump::new();
b.alloc_slice_fill_default::<u64>(usize::max_value());
}
Still, I'll do my best to figure out what's going on.
I believe this is a Valgrind bug.
Valgrind 3.13.0, which passed on master, reports:
--8686-- WARNING: unhandled amd64-linux syscall: 332
--8686-- You may be able to write your own handler.
--8686-- Read the file README_MISSING_SYSCALL_OR_IOCTL.
--8686-- Nevertheless we consider this a bug. Please report
--8686-- it at http://valgrind.org/support/bug_reports.html.
Valgrind 3.15.0, which fails here, reports:
==7387== Syscall param statx(file_name) points to unaddressable byte(s)
==7387== at 0x4B0989D: syscall (syscall.S:38)
==7387== by 0x18849B: statx (weak.rs:95)
[…]
==7387== Address 0x0 is not stack'd, malloc'd or (recently) free'd
==7387==
==7387== Syscall param statx(buf) points to unaddressable byte(s)
==7387== at 0x4B0989D: syscall (syscall.S:38)
==7387== by 0x18849B: statx (weak.rs:95)
[…]
==7387== Address 0x0 is not stack'd, malloc'd or (recently) free'd
The Valgrind 3.16.0 changelog (found here) says:
- ==================== FIXED BUGS ==================== […] n-i-bz sys_statx: don't complain if both |filename| and |buf| are NULL. […]
Valgrind 3.14.0 (same page):
- ==================== FIXED BUGS ==================== […] 392118 unhandled amd64-linux syscall: 332 (statx) […]
Merged in #94, along with the updated valgrind suppressions. Thanks again @Tamschi!
Thank you too, @fitzgen, for the help and criticism, and of course for accepting the feature request!
I indirectly mentioned this before, but succeeding with this PR gives me some piece of mind regarding where I'm going with my project's implementation details, so I'm really glad it happened. I hope the added functions will come in handy for others too, of course.
(I updated this opening comment to reflect the final state of the pull request. See its edit history for earlier versions.)
Changes
This pull requests adds new methods to
Bump
according to the following pattern:try_alloc
alloc
¹try_alloc_with
alloc_with
¹try_alloc_try_with
alloc_try_with
¹ already exists
The
try_alloc…
methods supplement the existingtry_alloc_layout
with generic equivalents that can be consumed more flexibly in safe Rust. The…_try_with
methods take a fallible initialiser and return anyErr
payload it creates by value, also attempt to undo theResult
's bump arena allocation in that process.The pull request also contains relevant tests and benches for each code path (mostly copies of
alloc_with.rs
), but I wasn't sure how to check for the deallocation from outside.Motivation
alloc_try_with
lets consumers use the plain Rust error escalation pattern with?
when constructing instances in place:It would be fairly cumbersome and require
unsafe
for consumers to implement this using the plainalloc_with
method due to the latter's&mut Result<_, _>
return type with the same kind of closure. It also wouldn't be possible for a consumer to undo the allocation as well, sinceAlloc::dealloc
won't free extra padding.alloc_try_with
is still a wrapper aroundalloc_with
, but exposes a safe API and is thorough with the deallocation.