Store LiveLoans more densely packed - #161850
Conversation
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Store LiveLoans more densely packed
This comment has been minimized.
This comment has been minimized.
|
Actually, this might be better stored with dimensions swapped after all. I didn't count it precisely yet, but it looks like there's usually more zeros at a few columns on the right, while last row There's also the tradeoff with iteration, though - points are usually assigned in succession (i.e. the iteration is Also , storing this as something packed in the spirit of |
|
Finished benchmarking commit (2b3c53b): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 3.1%, secondary 7.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.4%, secondary -4.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 475.228s -> 473.78s (-0.30%) |
| pub(crate) struct LiveLoans { | ||
| num_points: usize, | ||
| // This matrix always has more rows (PointIndex) than columns (BorrowIndex), so we store | ||
| // it row major to save memory when later columns are unused |
There was a problem hiding this comment.
I don't think this is row-major actually, I never remember which is which, I'll fix up the comment after review.
There was a problem hiding this comment.
Yes, this is column-major. I'm not sure being technical here helps: I'd just say we store as a flat matrix grouped by columns.
There was a problem hiding this comment.
We don't need to mention row-major or column-major, yes. We do need to better describe how the data is stored though, a growable bitset of usizes is not clear enough.
|
|
||
| pub(crate) type LiveLoans = SparseBitMatrix<PointIndex, BorrowIndex>; | ||
| #[derive(Clone)] | ||
| pub(crate) struct LiveLoans { |
There was a problem hiding this comment.
Not sure if it's worth making this a broadly available datastructure or if it's enough to keep it here for now?
There was a problem hiding this comment.
I think leaving this here for now is fine.
There was a problem hiding this comment.
It's only used in a small part of borrowck, and we will have to tweak it in the future anyways: the current behavior of filling this depends on how the graph is traversed. When traversing multiple loans at a time, the exact iteration and filling behavior will be different, and the best live loans representation will likely need to be reinvestigated. This matches your comment
There's also the tradeoff with iteration, though
| &lowered_constraints, | ||
| ); | ||
|
|
||
| let num_points = location_map.num_points(); |
There was a problem hiding this comment.
I'm not sure where is the right place to get this information from location_map. The way location_map is threaded through the code and the fact that it's in Rc makes it unclear at what point in time it is ready to use.
There was a problem hiding this comment.
This is fine. I was realizing too that currently things are all jumbled up and it might make sense to take a cleanup pass at some point.
I end up touching this in #161938 anyways, so not a big deal.
There was a problem hiding this comment.
This general shape is good and in line with what I'm already working on.
I think the key question here is if we want a DenseBitSet, GrowableBitSet, or MixedBitSet. We know the max size up front, and I doubt that later-indexed borrows go unused, so I doubt GrowableBitSet makes sense. MixedBitSet may be good if there are memory savings can outweigh the additional computational overhead.
(edit: local measurements have DenseBitSet better than the other two pretty convincingly)
I'm fine with whatever we decide here, if we have numbers that justify a benefit over the status quo - we can always optimize more later. (I think DenseBitSet is a safe choice for now.)
r=me with review addressed
| num_points: usize, | ||
| // This matrix always has more rows (PointIndex) than columns (BorrowIndex), so we store | ||
| // it row major to save memory when later columns are unused | ||
| flat_matrix: GrowableBitSet<usize>, |
There was a problem hiding this comment.
This could/should be a DenseBitSetof num_points * borrow_set.len()? I'm not sure that making this growable will give us meaningful memory benefits.
There was a problem hiding this comment.
I think just converting this into a dense, flat set gives significant performance wins.
There was a problem hiding this comment.
(That is also what I'm getting to, independently.)
There was a problem hiding this comment.
Possibly, using a MixedBitSet/ChunkedBitSet could have a better chance at reducing memory overhead when points for a given borrow are stored next to each other, since I generally would expect loans to occupy some subset of the fn.
In practice, I'm not sure this actually will end up being very helpful. Since I think we want to optimize for speed here, not memory. And these two will have a much larger computational overhead.
| pub(crate) struct LiveLoans { | ||
| num_points: usize, | ||
| // This matrix always has more rows (PointIndex) than columns (BorrowIndex), so we store | ||
| // it row major to save memory when later columns are unused |
There was a problem hiding this comment.
Yes, this is column-major. I'm not sure being technical here helps: I'd just say we store as a flat matrix grouped by columns.
|
|
||
| pub(crate) type LiveLoans = SparseBitMatrix<PointIndex, BorrowIndex>; | ||
| #[derive(Clone)] | ||
| pub(crate) struct LiveLoans { |
There was a problem hiding this comment.
I think leaving this here for now is fine.
| &lowered_constraints, | ||
| ); | ||
|
|
||
| let num_points = location_map.num_points(); |
There was a problem hiding this comment.
This is fine. I was realizing too that currently things are all jumbled up and it might make sense to take a cleanup pass at some point.
I end up touching this in #161938 anyways, so not a big deal.
|
r? me |
|
r? me |
|
Nice! Thanks for taking a look a this. It's unclear to me why a growable bitset would would be better in general. The density and loan reachability values should be dependent on the CFG and outlives graph shapes, and ofc the actual region liveness -- which feels unpredicatable to me, though I guess you could make assumptions that most code is straight-line code, and thus there could be a bias towards loans introduced late in the function being less likely to be live at the beginning of the function). I also don't believe our existing benchmarks are representative of the cases where this structure would matter in practice. I've seen big outliers where we could test this, and the other kinds of the inner bitset you're changing to a growable one. Do you see cpu/memory improvements to Having a points-per-loan bitset (instead of the current loans-per-point) is what the non-lazy loan traversal used to have as well. I'm pretty sure we'll need to do something cleverer once we propagate multiple loans at a time, to be able to exploit sparseness better for both the CFG points and loans themselves. |
Like I said in #161850 (comment), it's probably not better if we store it like this, because every borrow has at least one 1 point set, so this allocates almost always close to what I made the assumption from printing out dimensions, but later when I actually printed out the whole matrices, it became clear that we'd save more space if we flip rows and columns, which would match the assumption of the original code (btw. I assigned you (lqd) because you intruduced the
I don't have these set up locally, but I can try later. Jack has http IIRC, so I assume he tested on that. I test on I attached the print outs for serde if you want to get a quick idea for how these look like. I think it's pretty clear that there's more room for improvement (probably something with intervals). |
This comment has been minimized.
This comment has been minimized.
|
✌️ @panstromek, you can now approve this pull request! If @jackh726 told you to " |
No worries, I will do so myself tomorrow with the try build artifacts.
I had already started looking at this before you removed me, but will wait for the perf run results and the quick tests I mentioned above yes. |
|
Finished benchmarking commit (a63776d): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -3.0%, secondary 4.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.2%, secondary -4.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 475.395s -> 474.673s (-0.15%) |
|
Looking better than the previous run, and even better in the benchmarks I mentioned:
We can slightly improve the comments in the future, and this is already great as-is 🚀 @bors r=lqd,jackh726 |
This comment has been minimized.
This comment has been minimized.
01a08cb to
e515460
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors rollup=iffy |
Rollup merge of #161850 - panstromek:SparseBitMatrix, r=lqd,jackh726 Store LiveLoans more densely packed `LiveLoans` originally used `SparseBitMatrix<PointIndex, BorrowIndex>` in a way that didn't use "the sparseness" very effectively. The borrow dimension is usually very low (<10 bits 90% of the time), while the point dimension is typically order of magnitude higher. This means that we used to allocate a bunch of 32-byte `DenseBitSet`s that only point to a few bits in heap-allocated `u64`. We only saved some memory by not allocating the whole `PointIndex` range, but we still often need to allocatate most of it. This PR changes the representation to use a single flat representation with `GrowableBitSet` storing all bits in a single Vec. There's still a room for improvement but this should be a positive step forward. r? lqd
|
Note This PR was benchmarked as part of triage of its containing rollup: triage URL. Finished benchmarking commit (a9c8aa3): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.7%, secondary 0.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.3%, secondary 5.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: missing data |
View all comments
LiveLoansoriginally usedSparseBitMatrix<PointIndex, BorrowIndex>in a way that didn't use "the sparseness" very effectively. The borrow dimension is usually very low (<10 bits 90% of the time), while the point dimension is typically order of magnitude higher.This means that we used to allocate a bunch of 32-byte
DenseBitSets that only point to a few bits in heap-allocatedu64. We only saved some memory by not allocating the wholePointIndexrange, but we still often need to allocatate most of it.This PR changes the representation to use a single flat representation with
GrowableBitSetstoring all bits in a single Vec. There's still a room for improvement but this should be a positive step forward.r? lqd