Minor sharing-map improvements, used to avoid copying in value_sett #4539
Conversation
smowton
requested review from
chrisr-diffblue,
kroening,
martin-cs,
owen-mc-diffblue,
peterschrammel,
pkesseli and
tautschnig
as code owners
There was a problem hiding this comment.
Looks ok to me, except:
- It would be good for @danpoe to take a look if he's available.
- The last commit includes a lot of whitespace changes.
- Any data on how much of a performance difference this makes?
|
I verbally ran these by @danpoe as I went but yes please do take a look. The whitespace is unavoidable per clang-format I'm afraid, due to a 2-char indent difference between iterate-with-lambda and a for-loop. |
There was a problem hiding this comment.
✔️
Passed Diffblue compatibility checks (cbmc commit: 8118903).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/108399530
| leaft *lp = cp->find_leaf(k); | ||
| PRECONDITION(lp != nullptr); // key must exist in map | ||
|
|
||
| lp->mutate_value(mutator); |
There was a problem hiding this comment.
Can we add a debugging check here that tests if the mutated element is different from the old element? (if fail_if_equal is set, similar to the invariant in replace())
|
We should also update the documentation at the top of |
|
@tautschnig @danpoe conducted a quick micro-benchmark: Procedure: populate a value-set with 100K keys, each with 10 values. Modify half the keys by adding 1 extra value, then make-union the two value sets. Union operation took ~300ms with these changes, or ~750ms without them. The direction of the union made little difference (in one direction it would be a no-op; in one direction it would add a single value to 50k entries), suggesting the cost of editing 50k keys in place is small compared to the cost of copying 100k values out in order to edit them. |
smowton
force-pushed
the
smowton/feature/sharing-map-improvements
branch
from
8118903 to
927778e
Compare
|
@tautschnig @danpoe you might want to re-review; I made @danpoe's requested changes re: checking the update makes a change; this required me to add a flag so invariants can throw exceptions instead of aborting (so I could unit test them) and I also fixed |
Just saves repeating the if-has-key-then-erase pattern
This is generally a dangerous thing to do, but is useful for unit testing
This permits an in-place update, avoiding needless copy-out / mutate / move-in cycles for expensive-to-copy value types without leaking a non-const reference to a value.
This gives a simple const iterator without copying the whole dataset.
The make_union and update_entry maps are switched from a copy -> mutate -> move-if-changed workflow to compare-in-place -> update-in-place-if-needed one. This will be faster so long as map iteration and inspection is faster than copying. The output routine simply avoids a needless temporary vector.
smowton
force-pushed
the
smowton/feature/sharing-map-improvements
branch
from
927778e to
ff16508
Compare
There was a problem hiding this comment.
✔️
Passed Diffblue compatibility checks (cbmc commit: ff16508).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/108601221
This adds
sharing_mapt::erase_if_exists,::iterateand::update, and uses them to avoid copying entries invalue_sett. Thevalue_settchanges are much easier to review if you diff without whitespace sensitivity.Note
erase_if_existsisn't used in this PR (it's used out-of-tree), but is also the most simple of the additions.