Update ModuleSummaryIndexBitcodeReader::makeCallList reserve amount #95461
Conversation
Tighten the reserve() to `Record.size() / 2` instead of `Record.size()` in the HasProfile/HasRelBF cases (or even / 3 for IsOldProfileFormat). This reduces peak memory during ThinLTO indexing by ~3% in one example. Alternatively, we could reserve and then shrink_to_fit(), but seemed like we should just reserve the right amount (though that is a little more complex code).
and add shrink_to_fit in case of drift in the future (should be a no-op in the exact/common case).
|
@minglotus-6 would you be able to review this? (noticed this while looking at indexing memory usage) Thanks! |
| // Edge. Otherwise, conservatively reserve up to Record.size and later | ||
| // shrink_to_fit when we are done (and shrink_to_fit for the exact | ||
| // case should be a no-op). | ||
| if (!IsOldProfileFormat && (HasProfile || HasRelBF)) { |
There was a problem hiding this comment.
I don't think we should be optimizing for the old profile format which is very legacy at this point. I think the shrink_to_fit is only needed in that case - is that correct?
There was a problem hiding this comment.
Thanks for taking a look! Yes, shrink_to_fit is only needed in the legacy case right now.
A reason I had added it was as a safeguard in case the format changed in the future, but the reserve() parts weren't updated. Then it would provide a bit of protection.
Otherwise, I am happy to remove the shrink_to_fit too.
There was a problem hiding this comment.
Ok, removed the shrink_to_fit and simplified the comment a bit.
No worries, thanks for taking a look and helping add reviewers! |
| // case should be a no-op). | ||
| if (!IsOldProfileFormat && (HasProfile || HasRelBF)) { | ||
| Ret.reserve(Record.size() / 2); | ||
| } else |
There was a problem hiding this comment.
In this case the braces for the "then" branch can be removed per https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
We aren't interested in optimizing for the old formats.
…lvm#95461) Tighten the reserve() to `Record.size() / 2` instead of `Record.size()` in the HasProfile/HasRelBF cases. For the uncommon old profile format cases we leave it as is, but those should be rare and not worth optimizing. This reduces peak memory during ThinLTO indexing by ~3% in one example. Alternatively, we could make the branching for reserve more complex and try to cover every case.
Tighten the reserve() to
Record.size() / 2instead ofRecord.size()in the HasProfile/HasRelBF cases. For the uncommon old profile format cases we leave it as is, but those should be rare and not worth optimizing.This reduces peak memory during ThinLTO indexing by ~3% in one example.
Alternatively, we could make the branching for reserve more complex and try to cover every case.