Insert without ever opening a gap of raw memory - #78
Merged
Conversation
Fixes #74, and removes the family of problems it belongs to. make_uninitialized_space_new() relocated the elements into fresh storage in two steps and only recorded the size once both had finished. A move throwing in the second step left whatever the first one had already moved unreachable and unreleased: target's size was still zero, so its destructor freed the allocation without touching them. That leak was the third of its kind, after #68 and #74's own siblings, and they all came from the same design. Making space shifted the elements from pos out of the way and immediately counted the resulting hole in size(), so until the caller had constructed into it the container was claiming memory that held no elements. Every constructor that could throw then needed a rollback to close the hole again, and the rollback is the part that kept being wrong. insert_n() threads the new elements through the shift instead, the way std::vector does: what lands past the old end is constructed, what lands on an element that is still there is assigned over. There is no hole at any point, so nothing has to be undone, and size() only ever grows by a step that has already happened. Growing still builds the result in a separate allocation, where the constructed part genuinely is not a prefix, and that one place now spells its cleanup out with a scope guard. Assigning rather than destroying and reconstructing is also cheaper for types that can reuse their buffer. Instruction counts on gcc 16 -O3: fill_front int 184 -> 161, fill_front std::string 27092 -> 21611, fill_random std::string 13840 -> 11809. What it costs is a rollback that was never promised. An insert of several elements that has to shift them over live ones can now fail part way and leave the container with the right number of elements but no promise about which, which is the basic guarantee the standard asks for and what std::vector does. Everything before pos is still untouched. The paths that do not assign over live elements are unaffected: growing cannot lose the original, and a single element goes through emplace(), which builds it before anything is touched -- so insert(pos, value) now goes there too, which also lets it take one of our own elements without copying it aside. std::rotate was tried for this and is a trap: libstdc++ answers it with a block swapping algorithm, and three moves per swap made fill_front on std::string twice as expensive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
No behaviour change, all of it review fallout. svector.h: * assign() never had a non-zero offset at any call site, so drop the parameter and the std::next() that was provably a no-op with it * insert_n_new() took a size its caller had already read off it * the placer doc described insert_n()'s algorithm rather than the placer, and insert_n() then said the same thing twice more. Say it once, where it is implemented * insert_n() claimed growing was all or nothing. It is only as all or nothing as relocating a T is: a move constructor that throws part way leaves our own elements moved from, and then all that survives is that nothing leaks * is_reference_into_self()'s doc read as a universal rule while two of its callers deliberately no longer follow it. Say which two ways there are to get a value out of the way, and that this is only for one of them Tests: * one budget instead of four identical statics, spent through tick() * the arrange/throw/verify block was written out three times, along with three copies of the growth condition it dispatches on. It is one helper now, so "grew, therefore nothing happened" is defined once * a comment still explained issue #69 in terms of make_uninitialized_space(), and several still described the rotate that was tried and rejected * ThrowOnCopy::operator== was dead Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Fixes #74, and removes the family of problems it belongs to.
The leak
make_uninitialized_space_new()relocated the elements into fresh storage in two steps and only recorded the size once both had finished. A move throwing in the second step left whatever the first one had already moved unreachable and unreleased —target's size was still zero, so its destructor freed the allocation without touching them.Why the fix is not a
try/catchThat leak was the third of its kind. #68 and this one both came from the same design: making space shifted the elements from
posout of the way and immediately counted the resulting hole insize(), so until the caller had constructed into it the container was claiming memory that held no elements. Every constructor that could throw then needed a rollback to close the hole again — and the rollback is the part that kept being wrong.insert_n()threads the new elements through the shift instead, the waystd::vectordoes: what lands past the old end is constructed, what lands on an element that is still there is assigned over. There is no hole at any point, so nothing has to be undone, andsize()only ever grows by a step that has already happened.shift_right(),make_uninitialized_space(),fill_uninitialized_space()andclose_gap()are all gone.Growing still builds the result in a separate allocation, where the constructed part genuinely is not a prefix, and that one place now spells its cleanup out with a scope guard.
It is also faster
Assigning rather than destroying and reconstructing lets types reuse their buffer. Instruction counts, gcc 16 -O3, two binaries built from the two headers and run interleaved:
fill_frontintfill_frontstd::stringfill_randomstd::stringstd::rotatewas tried for this and is a trap: libstdc++ answers it with a block swapping algorithm, and three moves per swap madefill_frontonstd::stringtwice as expensive. A hand written cycle rotate fixed that and costint55x, because the shift stops being amemmove.What it costs
A rollback that was never promised. An insert of several elements that has to shift them over live ones can now fail part way and leave the container with the right number of elements but no promise about which — the basic guarantee the standard asks for, and what
std::vectordoes. Everything beforeposis still untouched.The paths that do not assign over live elements are unaffected:
insert_throwing_move_while_growing_does_not_leakasserts.emplace(), which builds it before anything is touched — soinsert(pos, value)now goes there too, which as a side effect lets it take one of our own elements without copying it aside firsttest/unit/insert_exception_safety.cppsays which is which, and the element types now throw on copy assignment as well as copy construction, without which half the new paths never see a failure.Verification
insert_throwing_move_while_growing_does_not_leakfails with 69,536 bytes leaked in 1,696 allocations when the scope guard is removedKnown trade: code size
Reviewing this turned up one measured regression I am not fixing here. The shift is now instantiated once per
(direction, placer)pair, where the oldmake_uninitialized_space<D>was two instantiations shared by every insert form. Base cost for a single insert form is unchanged (9,816 B vs 10,014 B on main), but each additional form costs ~8.2 KB instead of ~2.8 KB — 2.1x total.textforstd::stringand 4.3x forintacross six insert forms in one TU.Dropping the
directionparameter halves it (string -29%, int -50%) but costs theinthot path 5.7%, and two narrower splits measured worse than the status quo. Filed separately rather than churning the diff.