Replace dependency on boost with a custom small vector #13716

lhecker · 2022-08-10T13:39:21Z

This replaces ~70k LOC (parts of boost, 1/4th of the code in this project)
with ~700 LOC (small_vector.h). By replacing boost, we simplify future
maintenance and improve compile times.

Validation Steps Performed

New and existing unit tests are ok ✅
Various common VT applications run fine in debug mode OpenConsole ✅

DHowett · 2022-08-10T21:02:04Z

Hell yeah.

Why is this difficult to review?

lhecker · 2022-08-10T21:04:01Z

Why is this difficult to review?

The code isn't that trivial tbh. I honestly thought it was pretty solid when I opened the PR, but as you can see, the tests disagree and this code remains buggy.

zadjii-msft · 2022-08-10T21:11:10Z

Yea I mean, small_vector.h is the hard part of this review. There's like what, 11 other relevant changes, and then 450 files of "delete boost" which I'm just gonna skip over. If you get the tests fixed, then hell yea let's ship this. Lets us close out the internal bugs we have about "switch boost to vcpkg" (or whatever they are) too.

DHowett

Add some tests, answer the Qs, and we'll be golden!

src/inc/til.h

src/inc/til/small_vector.h

DHowett · 2022-08-15T22:21:27Z

.github/actions/spelling/allow/allow.txt

@@ -57,6 +57,7 @@ locl
 lorem
 Lorigin
 maxed
+minimalistic


i think the word is just "minmal" :D

I'm fine with this.jpg. Minimalistic sounds like something I'd say.

src/inc/til/small_vector.h

zadjii-msft · 2022-08-24T16:54:59Z

Do we think I'm actually qualified to review the implementation of a basic collection like that in c++? Or should I lean on the fact that the tests passed and nothing seems to explode? Otherwise @miniksa is usually a bit more fluent in the idomatic way of implementing something like this, and might be a better reviewer for https://github.com/microsoft/terminal/pull/13716/files#diff-396eb9779e7a38e7354acd9c04395d1fb502e991b843ead351c1312156b14a5a

DHowett · 2022-08-24T17:35:10Z

This is in today's bug bash build as well!

miniksa · 2022-08-25T01:39:58Z

Do we think I'm actually qualified to review the implementation of a basic collection like that in c++? Or should I lean on the fact that the tests passed and nothing seems to explode? Otherwise @miniksa is usually a bit more fluent in the idomatic way of implementing something like this, and might be a better reviewer for https://github.com/microsoft/terminal/pull/13716/files#diff-396eb9779e7a38e7354acd9c04395d1fb502e991b843ead351c1312156b14a5a

I can put it on my list for tomorrow.

miniksa

Very exciting to be losing boost. Looks pretty good. I didn't have any blocking comments.

miniksa · 2022-08-25T15:26:11Z

.github/actions/spelling/allow/allow.txt

@@ -57,6 +57,7 @@ locl
 lorem
 Lorigin
 maxed
+minimalistic


I'm fine with this.jpg. Minimalistic sounds like something I'd say.

src/buffer/out/CharRow.hpp

miniksa · 2022-08-25T15:29:14Z

src/inc/til/small_vector.h

+// small_vector::_data can reference both non-owned (&_buffer[0]) and owned (new[]) data.
+#pragma warning(disable : 26402) // Return a scoped object instead of a heap-allocated if it has a move constructor (r.3).
+// small_vector manages a _capacity of potentially uninitialized data. We can't use regular new/delete.
+#pragma warning(disable : 26409) // Avoid calling new and delete explicitly, use std::make_unique<T> instead (r.11).
+// That's how the STL implemented their std::vector<>::iterator. I simply copied the concept.
+#pragma warning(disable : 26434) // Function '...' hides a non-virtual function '...'.
+// Functions like front()/back()/operator[]() are explicitly unchecked, just like the std::vector equivalents.
+#pragma warning(disable : 26446) // Prefer to use gsl::at() instead of unchecked subscript operator (bounds.4).
+// small_vector::_data references potentially uninitialized data and so we can't pass it regular iterators which reference initialized data.
+#pragma warning(disable : 26459) // You called an STL function '...' with a raw pointer parameter at position '...' that may be unsafe ... (stl.1).
+// small_vector::_data references potentially uninitialized data and so we can't pass it regular iterators which reference initialized data.
+#pragma warning(disable : 26481) // Don't use pointer arithmetic. Use span instead (bounds.1).


This has to be the most comprehensive explanation of warning suppressions I've ever seen. Fantastic!

miniksa · 2022-08-25T15:31:12Z

src/inc/til/small_vector.h

+
+namespace til
+{
+    // This class was adopted from std::span<>::iterator.


I would have written "stolen shamelessly" instead of "adopted". But I guess your wording is technically a better idea.

miniksa · 2022-08-25T15:34:03Z

src/inc/til/small_vector.h

+            reserve(other._size);
+
+            std::uninitialized_copy(other.begin(), other.end(), _uninitialized_begin());
+            _size = other._size;


Very good call with reporting empty on failure.

miniksa · 2022-08-25T15:35:47Z

src/inc/til/small_vector.h

+        // NOTE: If an exception is thrown while copying, the vector is left empty.
+        small_vector& operator=(const small_vector& other)
+        {
+            clear();


I was about to ask why you needed std::destroy for the move constructor but not for here, but I suppose it's because the destructors will be naturally called as you're clear()ing. Also I bet I could answer this myself if I scrolled further down.

clear() only destroys the data, but doesn't free the allocated memory.
The copy constructor wants exactly that: It wants to reuse the existing memory. But the move constructor needs to free its own memory, since it'll take ownership of the memory of the other vector.

Ah, I was thinking about calling the destructors of the objects inside when I wrote this comment not necessarily about using the memory space.

miniksa · 2022-08-25T15:38:25Z

src/inc/til/small_vector.h

+            if (_capacity != N)
+            {
+                _deallocate(_data);
+            }
+
+            if (other._capacity == N)
+            {
+                _data = &_buffer[0];
+                _capacity = N;
+                _size = other._size;
+                // The earlier static_assert(std::is_nothrow_move_constructible_v<T>)
+                // ensures that we don't exit in a weird state with invalid `_size`.
+#pragma warning(suppress : 26447) // The function is declared 'noexcept' but calls function '...' which may throw exceptions (f.6).
+                std::uninitialized_move(other.begin(), other.end(), _uninitialized_begin());
+                std::destroy(other.begin(), other.end());
+            }


You're going to want to explain this. Why is it better to do it this way over just having the pointers always be moved into this one and calling it a day. I presume it's an optimization of some sort, but I don't know what it is. Comments?

You mean why I check if (other._capacity == N) here and do this extra branch?
If the other vector is "small" (= data is in the _buffer array) its _capacity will equal N. But unlike a heap allocated pointer we can't just "move" a stack allocated array - we can only copy it. And that's why use std::uninitialized_move here to create a copy of the other vector's _buffer.

Ah yes yes. It's literally the entire optimization. Brain fart. Maybe just leave a small comment on the branch to help remind goofballs like me.

src/inc/til/small_vector.h

DHowett · 2022-09-02T22:23:06Z

@lhecker are the TODOs to-done?

lhecker · 2022-09-02T23:22:27Z

@DHowett No I haven't worked on this yet. I was planning to continue with this PR once we forked off 1.16.

lhecker · 2022-09-13T20:35:41Z

@DHowett I've just pushed some final fixes and improvements and I think this PR is now ready to be merged. If possible I'd love if you could give the last commit another review.

DHowett · 2022-09-13T20:46:27Z

src/inc/til/small_vector.h

+                // This line is noexcept:
+                // It can't throw because of the earlier static_assert(std::is_nothrow_move_assignable_v<T>).
+                const auto displacement = std::min(count, moveable);
+                std::uninitialized_move(end() - displacement, end(), _uninitialized_begin() + (new_size - displacement));


do we have tests exercising this code? validating it?

Yes, parts of the Basic tests cover this.

DHowett

I think i understand!

ghost · 2022-09-13T21:32:31Z

Hello @DHowett!

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.

lhecker marked this pull request as draft August 10, 2022 13:39

zadjii-msft added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Aug 10, 2022

DHowett approved these changes Aug 10, 2022

View reviewed changes

src/inc/til.h Show resolved Hide resolved

src/inc/til/small_vector.h Outdated Show resolved Hide resolved

src/inc/til/small_vector.h Outdated Show resolved Hide resolved

lhecker force-pushed the dev/lhecker/small-vector branch from 83a3b2c to 9f9f704 Compare August 12, 2022 17:35

lhecker requested a review from DHowett August 12, 2022 17:36

lhecker added 2 commits August 12, 2022 19:40

Replace dependency on boost with a custom small vector

b0f8456

Fix various small_vector bugs

efa468b

lhecker force-pushed the dev/lhecker/small-vector branch from 9f9f704 to efa468b Compare August 12, 2022 17:41

This comment was marked as outdated.

Sign in to view

Fix x86 build, AuditMode, spell checks

85d6c20

This comment was marked as outdated.

Sign in to view

minimalistic is a word

6e0a33b

lhecker marked this pull request as ready for review August 13, 2022 18:17

j4james reviewed Aug 14, 2022

View reviewed changes

src/inc/til/small_vector.h Outdated Show resolved Hide resolved

lhecker added 3 commits August 14, 2022 17:05

Un-_Uglify names, Add _Verify_range, Swap vector with array iterator

c9ccf71

Terminal's first use of C++20 concepts

86f0018

Concepts are too powerful

4adc676

j4james reviewed Aug 14, 2022

View reviewed changes

src/inc/til/small_vector.h Outdated Show resolved Hide resolved

lhecker added 3 commits August 14, 2022 23:45

Fix erase bug

af97cac

Provide basic exception safety

fa79ae3

Deduplicate push_back, Protect against arithmetic overflow

b92485e

DHowett reviewed Aug 15, 2022

View reviewed changes

src/inc/til/small_vector.h Outdated Show resolved Hide resolved

DHowett added a commit that referenced this pull request Aug 23, 2022

PRE-MERGE #13716 Replace dependency on boost with a custom small vector

85024d6

miniksa approved these changes Aug 25, 2022

View reviewed changes

zadjii-msft added this to the Terminal v1.17 milestone Aug 25, 2022

lhecker added 2 commits September 13, 2022 17:47

Merge remote-tracking branch 'origin/main' into dev/lhecker/small-vector

2bfcb85

Improve natvis, Allow insert to inline

6c52746

Fix a natvis whoops

c5bf512

DHowett reviewed Sep 13, 2022

View reviewed changes

DHowett approved these changes Sep 13, 2022

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 13, 2022

Fix AuditMode errors

e331524

ghost merged commit 71fc4b1 into main Sep 14, 2022

ghost deleted the dev/lhecker/small-vector branch September 14, 2022 13:57

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace dependency on boost with a custom small vector #13716

Replace dependency on boost with a custom small vector #13716

lhecker commented Aug 10, 2022 •

edited

Loading

DHowett commented Aug 10, 2022

lhecker commented Aug 10, 2022

zadjii-msft commented Aug 10, 2022

DHowett left a comment

This comment was marked as outdated.

This comment was marked as outdated.

DHowett Aug 15, 2022

miniksa Aug 25, 2022

zadjii-msft commented Aug 24, 2022 •

edited

Loading

DHowett commented Aug 24, 2022

miniksa commented Aug 25, 2022

miniksa left a comment

miniksa Aug 25, 2022

miniksa Aug 25, 2022

miniksa Aug 25, 2022

miniksa Aug 25, 2022

miniksa Aug 25, 2022

lhecker Aug 25, 2022 •

edited

Loading

miniksa Aug 26, 2022

miniksa Aug 25, 2022

lhecker Aug 25, 2022

miniksa Aug 26, 2022

DHowett commented Sep 2, 2022

lhecker commented Sep 2, 2022

lhecker commented Sep 13, 2022

DHowett Sep 13, 2022

lhecker Sep 13, 2022

DHowett left a comment

ghost commented Sep 13, 2022

Replace dependency on boost with a custom small vector #13716

Replace dependency on boost with a custom small vector #13716

Conversation

lhecker commented Aug 10, 2022 • edited Loading

Validation Steps Performed

DHowett commented Aug 10, 2022

lhecker commented Aug 10, 2022

zadjii-msft commented Aug 10, 2022

DHowett left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft commented Aug 24, 2022 • edited Loading

DHowett commented Aug 24, 2022

miniksa commented Aug 25, 2022

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

Choose a reason for hiding this comment

lhecker Aug 25, 2022 • edited Loading

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

DHowett commented Sep 2, 2022

lhecker commented Sep 2, 2022

lhecker commented Sep 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

ghost commented Sep 13, 2022

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.

lhecker commented Aug 10, 2022 •

edited

Loading

zadjii-msft commented Aug 24, 2022 •

edited

Loading

lhecker Aug 25, 2022 •

edited

Loading

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.