Introduce til::rle - a run length encoded vector #10099

lhecker · 2021-05-14T21:51:54Z

Summary of the Pull Request

Introduces til::rle, a vector-like container which stores elements of
type T in a run length encoded format. This allows efficient compaction
of repeated elements within the vector.

References

[Epic] Text Buffer rewrite #8000 - Supports buffer rewrite work. A re-use of til::rle will be
useful as a column counter as we pursue NxM storage and presentation.
Processing large data is extremely slow (in VMware) #3075 - The new iterators allow skipping forward by multiple units,
which wasn't possible under TextBuffer-/OutputCellIterator.
Additionally it also allows a bulk insertions.
Give til::bitmap custom allocator support and add til::pmr::bitmap #8787 and Rendering performance of chafa is very slow #410 - High probability this should be pmr-ified
like bitmap for things like chafa and cacafire
which are changing the run length frequently.

PR Checklist

Closes Abstract AttrRow as til::rle<T> #8741
I work here.
Tests added.
Tests passed.

Validation Steps Performed

Ran cacafire in OpenConsole.exe and it looked beautiful
Ran new suite of RunLengthEncodingTests.cpp

Co-authored-by: Michael Niksa miniksa@microsoft.com

## Summary of the Pull Request Introduces `til::rle`, a vector-like container which stores elements of type T in a run length encoded format. This allows efficient compaction of repeated elements within the vector. ## References * #8000 - Supports buffer rewrite work. A re-use of `til::rle` will be useful as a column counter as we pursue NxM storage and presentation. * #3075 - The new iterators allow skipping forward by multiple units, which wasn't possible under `TextBuffer-/OutputCellIterator`. Additionally it also allows a bulk insertions. * #8787 and #410 - High probability this should be `pmr`-ified like `bitmap` for things like `chafa` and `cacafire` which are changing the run length frequently. ## PR Checklist * [x] Closes #8741 * [x] I work here. * [x] Tests added. * [x] Tests passed. ## Validation Steps Performed * [x] Ran `cacafire` in `OpenConsole.exe` and it looked beautiful * [x] Ran new suite of `RunLengthEncodingTests.cpp` Co-authored-by: Michael Niksa <miniksa@microsoft.com>

DHowett · 2021-05-18T22:38:36Z

@msftbot make sure @miniksa signs off on this

ghost · 2021-05-18T22:38:42Z

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett

It's perfect, and I love it. I can only find fault in a comment, and I did attempt to understand the algorithm. I've kicked the tires with my buffer implementation, as well, and it looks great. Excellent work!

src/til/ut_til/RunLengthEncodingTests.cpp

DHowett · 2021-05-18T22:51:49Z

src/inc/til/rle.h

+    template<typename T, typename S = std::size_t, std::size_t N = 1>
+    using small_rle = basic_rle<T, S, boost::container::small_vector<rle_pair<T, S>, N>>;
+#endif
+};


If you wanted to add one more class template (for fun!) i would suggest til::pmr::rle that uses a std::pmr::vector. However! basic_rle can't take an Allocator type, and this seems like an unnecessary cost for something we do not currently need. 😁

Ah, I meant "type alias" instead of "class template"

src/inc/til/rle.h

src/til/ut_til/RunLengthEncodingTests.cpp

miniksa

I am very very pleased. You took my start and you made it shine. I love it. Excellent work.

Just a few little comments to clean up that I'd like to see the answers for before I sign for merge.

src/inc/til/rle.h

miniksa · 2021-05-19T17:28:19Z

src/inc/til/rle.h

+    // rle_pair is a simple clone of std::pair, with one difference:
+    // copy and move constructors and operators are explicitly defaulted.
+    // This allows rle_pair to be std::is_trivially_copyable, if both T and S are.
+    // --> rle_pair can be used with memcpy(), unlike std::pair.


ooooh nifty.

src/inc/til/rle.h

miniksa · 2021-05-19T17:38:39Z

src/inc/til/rle.h

+            //
+            //
+            //
+            // MUST READ: How this function (mostly) works


I love this comment so much. It is exactly what I was hoping for.

miniksa · 2021-05-19T17:45:37Z

src/til/ut_til/RunLengthEncodingTests.cpp

+
+        VERIFY_ARE_EQUAL("1|3 3|2|1 1 1|5 5"sv, rle);
+        // empty
+        VERIFY_ARE_EQUAL(""sv, rle.slice(0, 0)); // begin


FYI, VERIFY_ARE_EQUAL takes a 3rd parameter which can be a string you want printed out in the log output so you can more easily tell which sub-test this is. (If you wanted to do that over the comments.)

miniksa · 2021-05-19T17:46:12Z

src/til/ut_til/RunLengthEncodingTests.cpp

+        using value_type = typename rle_vector::value_type;
+
+    public:
+        static bool AreEqual(const ::std::string_view& expected, const rle_vector& actual) noexcept


This is super elegant and I love it.

src/til/ut_til/RunLengthEncodingTests.cpp

lhecker · 2021-05-20T15:22:28Z

@miniksa @DHowett The most recent commit changes the way the iterator works: Instead of 1-based indices it's now based on 0-based ones. I felt that this fits better with the the container class itself, which is also written with 0-base indexes in mind. It also fixes a off-by-one error in operator-().

DHowett · 2021-05-20T15:56:32Z

Ooh, you made the static analyzer mad!

DHowett · 2021-05-20T15:57:23Z

src/inc/til/rle.h

-                operator-=(1);
+                if (_pos == 0)
+                {
+                    --_it;


Will the vector [debug] iterator catch the out of bounds move if the user is at [0]?

The std::vector one will. I'm not sure about the one in boost.
But this code is functionally equivalent to the operator+= implementation (just much simpler for inlining).

miniksa · 2021-05-20T16:03:48Z

@miniksa @DHowett The most recent commit changes the way the iterator works: Instead of 1-based indices it's now based on 0-based ones. I felt that this fits better with the the container class itself, which is also written with 0-base indexes in mind. It also fixes a off-by-one error in operator-().

Yeah that's probably wise. I don't know what drugs I was on when I wrote it 1-based. Thanks for fixing it.

miniksa

I'm good. Thanks for taking this over the finish line. It looks great. Minorly sad that some of the comments got removed from the iterators, but you're probably right to do so as they really weren't describing the "why".

ghost · 2021-05-20T17:19:09Z

Hello @lhecker!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2021-05-25T19:08:57Z

🎉Windows Terminal Preview v1.9.1445.0 has been released which incorporates this pull request.:tada:

Handy links:

lhecker requested review from DHowett and miniksa May 14, 2021 21:51

lhecker force-pushed the dev/lhecker/rle branch from 997fedd to e55a1ea Compare May 14, 2021 21:54

lhecker force-pushed the dev/lhecker/rle branch from e55a1ea to 4b0eeef Compare May 14, 2021 21:56

microsoft deleted a comment from github-actions bot May 14, 2021

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 18, 2021

DHowett approved these changes May 18, 2021

View reviewed changes

Address review comments

7fdfaa7

DHowett reviewed May 19, 2021

View reviewed changes

src/til/ut_til/RunLengthEncodingTests.cpp Outdated Show resolved Hide resolved

miniksa requested changes May 19, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels May 19, 2021

This comment has been minimized.

Sign in to view

Address reviewer comments

8a4039f

lhecker force-pushed the dev/lhecker/rle branch from d3eb250 to 8a4039f Compare May 20, 2021 15:23

DHowett approved these changes May 20, 2021

View reviewed changes

miniksa removed the AutoMerge Marked for automatic merge by the bot when requirements are met label May 20, 2021

miniksa approved these changes May 20, 2021

View reviewed changes

Improve documentation

a2ce9fc

lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 20, 2021

ghost merged commit a8e4bed into main May 20, 2021

ghost deleted the dev/lhecker/rle branch May 20, 2021 17:27

ghost mentioned this pull request May 25, 2021

Abstract AttrRow as til::rle<T> #8741

Closed

7 tasks

lhecker mentioned this pull request Jun 19, 2021

Extremely slow performance when processing virtual terminal sequences #10462

Open

lhecker mentioned this pull request Mar 24, 2024

Implement grapheme clusters #16916

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce til::rle - a run length encoded vector #10099

Introduce til::rle - a run length encoded vector #10099

lhecker commented May 14, 2021

DHowett commented May 18, 2021

ghost commented May 18, 2021

DHowett left a comment

DHowett May 18, 2021

DHowett May 18, 2021

miniksa left a comment

miniksa May 19, 2021

miniksa May 19, 2021

miniksa May 19, 2021

miniksa May 19, 2021

This comment has been minimized.

lhecker commented May 20, 2021

DHowett commented May 20, 2021

DHowett May 20, 2021

lhecker May 20, 2021

miniksa commented May 20, 2021

miniksa left a comment

ghost commented May 20, 2021

ghost commented May 25, 2021

Introduce til::rle - a run length encoded vector #10099

Introduce til::rle - a run length encoded vector #10099

Conversation

lhecker commented May 14, 2021

Summary of the Pull Request

References

PR Checklist

Validation Steps Performed

DHowett commented May 18, 2021

ghost commented May 18, 2021

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miniksa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

lhecker commented May 20, 2021

DHowett commented May 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miniksa commented May 20, 2021

miniksa left a comment

Choose a reason for hiding this comment

ghost commented May 20, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented May 25, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.