build a result unwrap_or's fallback lazily#592
Merged
Conversation
result `unwrap_or` evaluated its fallback before branching on ok/err. on the ok path the fallback was never used, so a heap default — `r.unwrap_or(bytes.empty())` is the everyday shape — was allocated and dropped every call, leaking one object per iteration. a loop over an ok result grew ~64 bytes an iteration without end. the optional `unwrap_or` already builds its default inside the none arm; the result path now does the same. on the ok path the fallback expression is never evaluated, which is both leak-free and the semantics you'd expect from a lazy-looking default. tests/cases/test_result_unwrap_or_fallback_leak drives 200k ok iterations through a heap-fallback unwrap_or and checks the footprint stays flat. it uses an inline (owned) operand on purpose: `unwrap_or` on a borrowed operand — a named result local, or a `T!` param — has a separate, pre-existing ownership bug (it shell-releases an operand its owner also releases) that is being fixed on its own; this test isolates the fallback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
summary
result
unwrap_orevaluated its fallback before branching on ok/err, so on theok path the fallback was built and then discarded — a leak of one object per
call for any heap default.
r.unwrap_or(bytes.empty())is the everyday shape,and a loop over an ok result grew ~64 bytes an iteration without bound.
the optional
unwrap_oralready builds its default inside the none arm; theresult path now matches. on the ok path the fallback expression is never
evaluated, which is leak-free and the semantics a lazy-looking default implies.
measured
a 200k-iteration loop through
make(i).unwrap_or(bytes.empty()).len(), okevery time: VmHWM ~21 mb before, ~2.5 mb after (flat).
what was tested
make bootstrap-verifygreen,make memcheckclean under valgrind includingthe new case.
tests/cases/test_result_unwrap_or_fallback_leakdrives the okpath 200k times and asserts the footprint stays flat; it is in MEMCHECK_CASES.
notes
the test uses an inline (owned) operand deliberately.
unwrap_oron a borrowedoperand — a named result local, or a
T!parameter — has a separate,pre-existing ownership bug: it shell-releases an operand whose owner also
releases it, a use-after-free that
PITH_STRUCT_FREELIST=0valgrind exposes.that is a distinct fix (it is shared with
!andcatchon a borrowed operand)and is being handled on its own; this PR isolates the fallback leak so the two
don't tangle.