Create more efficient diffs #140
Conversation
… alternative approach of sorting but maintaining original index.
| size_t s = 0; | ||
| size_t t = 0; | ||
|
|
||
| /// Generate diff_result that when `apply_diff(orig, diff_result)` will modify source to be equal to target. |
There was a problem hiding this comment.
You don't modify source any more.
| @@ -24,87 +34,138 @@ template <typename T, typename SizeType = size_t, template<typename Y, typename. | |||
| requires std::equality_comparable<T> && std::random_access_iterator<typename Container<T>::iterator> | |||
| class ordered_diff { | |||
| public: | |||
|
|
|||
| /// All indexes are in reference to the target. Apply in order: remove, insert, move. | |||
| /// remove_indexes are in reverse order so they can be applied directly, see apply_diff. | |||
There was a problem hiding this comment.
Maybe you can give a simple example here, if not too much trouble.
| auto targ_sorted = sort_input(target); | ||
|
|
||
| size_t orig_index = 0; | ||
| size_t cur_index = 0; |
| result.remove_indexes.emplace_back(orig_sorted[orig_index].second); | ||
| ++orig_index; | ||
| } else if (cmp == std::weak_ordering::greater) { | ||
| assert(targ_sorted[cur_index].second <= std::numeric_limits<SizeType>::max()); |
| } else { | ||
| cmp = (orig_sorted[orig_index].first <=> targ_sorted[cur_index].first); | ||
| } | ||
|
|
There was a problem hiding this comment.
You can add comment to each of the cmp ==
| } | ||
| } | ||
|
|
||
| std::ranges::sort(result.remove_indexes, [](auto&& lhs, auto&& rhs) { return lhs > rhs; }); // descending | ||
| std::ranges::sort(result.insert_indexes, [](auto&& lhs, auto&& rhs) { return lhs.first < rhs.first; }); |
There was a problem hiding this comment.
Why do insert_indexes and move_indexes need to be sorted?
| /// @return the modified container now equal to original target | ||
| /// @param diff the diff_result created from diff(orig, target), apply_diff(std::move(orig), diff_result) => target | ||
| /// @param container the orig of diff(orig, target) to modify using the diff_result to produce the target | ||
| /// @return the modified container now equal to target | ||
| template <typename X> | ||
| requires std::same_as<std::decay_t<X>, diff_result> | ||
| static Container<T> apply_diff(Container<T>&& container, X&& diff) { |
There was a problem hiding this comment.
Should container be sorted first?
It is also better to be named original or orig.
| for (SizeType index : diff.remove_indexes) { | ||
| container.erase(container.begin() + index + offset); | ||
| --offset; | ||
| // Remove from the container based on diff.remove_indexes which is descending order |
There was a problem hiding this comment.
Another possible way to do apply_diff is:
- Have a fresh target.
- Skip every index in
diff.remove_indexes - Insert every value in
diff.insert_indexesinto the target - Move value in diff.move_indexes to right location in target.
This way the erases/copies can be avoided, and seems less code.
| std::ranges::sort(result.insert_indexes, [](auto&& lhs, auto&& rhs) { return lhs.first < rhs.first; }); | ||
| std::ranges::sort(result.move_indexes, [](auto&& lhs, auto&& rhs) { return lhs.first < rhs.first; }); | ||
|
|
||
| // update move indexes |
There was a problem hiding this comment.
This section of updating move indexes is quite complicated. Can it somehow be avoided by tweaking other parts of the algorithm?
| @@ -24,87 +34,138 @@ template <typename T, typename SizeType = size_t, template<typename Y, typename. | |||
| requires std::equality_comparable<T> && std::random_access_iterator<typename Container<T>::iterator> | |||
| class ordered_diff { | |||
| public: | |||
|
|
|||
| /// All indexes are in reference to the target. Apply in order: remove, insert, move. | |||
There was a problem hiding this comment.
Isn't remove_indexes for original?
|
Note:start |
|
Not going to make release. May re-visit for a future consensus release. |
Use a more efficient diff for
finalizer_policyandproposer_policy.Resolves #5