Update til::bitmap to use dynamic_bitset<> + libpopcnt #5092

miniksa · 2020-03-23T23:23:23Z

This commit replaces std::vector<bool> with dynamic_bitset<> by
@pinam45 (https://github.com/pinam45/dynamic_bitset) and with
libpopcnt for high-performance bit counting by @kimwalisch
(https://github.com/kimwalisch/libpopcnt).

In support of performance, incremental rendering, and Terminal
"not speed enough" as well as my sanity relative to
std::vector<bool>
Tests updated and passed.
LICENSE, NOTICE, and provenance files updated.
I'm a core contributor. I discussed it with @DHowett-MSFT and
cleared the licensing checks before pulling this in.

Details `std::vector<bool>` provided by the Microsoft VC Runtime is

incapable of a great many things. Many of the methods you come to expect
off of std::vector<T> that are dutifully presented through the bool
variant will spontaneously fail at some future date because it decides
you allocated, resized, or manipulated the vector<bool> specialization
in an unsupported manner. Half of the methods will straight up not work
for filling/resizing in bulk. And you will tear your hair out as it will
somehow magically forget the assignment of half the bits you gave it
part way through an iteration then assert out and die.

As such, to preserve my sanity, I searched for an alternative. I came
across the self-contained header-only library dynamic_bitset by
@pinam45 which appears to do as much of boost::dynamic_bitset as I
wanted, but without including 400kg of boost libraries. It also has a
nifty optional dependency on libpopcnt by @kimwalisch that will use
processor-specific extensions for rapidly counting bits. @DHowett-MSFT
and I briefly discussed how nice popcnt would have been on
std::vector<bool> last week... and now we can have it. (To be fair, I
don't believe I'm using it yet... but we'll be able to easily dial in
til::bitmap soon and not worry about a performance hit if we do have
to walk bits and count them thanks to libpopcnt.)

This PR specifically focuses on swapping the dependencies out and
ingesting the new libraries. We'll further tune til::bitmap in future
pulls as necessary.

Validation

Ran the automated tests for bitmap.
Ran the terminal manually and it looks fine still.

…opcnt. Update NOTICE, include LICENSES, add all governance traceback. Include in common paths and common library imports.

DHowett-MSFT · 2020-03-23T23:27:06Z

src/inc/til/bitmap.h

            _dirty(fill ? sz : til::rectangle{}),
            _runs{}
        {
+            if (fill)


it looked to me like the ctor for dynamic bitset supported this still

nnnnnnn I had that and it wasn't working right. It took the long long value of the bool as 1 and splatted that in a block instead of filling the whole block with bits.

zadjii-msft

Looks fine to me, but you got a bunch of build breaks so gl;hf

zadjii-msft · 2020-03-24T12:53:07Z

src/common.build.pre.props

@@ -82,7 +82,7 @@
      <PrecompiledHeaderFile>precomp.h</PrecompiledHeaderFile>
      <ProgramDataBaseFileName>$(IntDir)$(TargetName).pdb</ProgramDataBaseFileName>
      <DebugInformationFormat>ProgramDatabase</DebugInformationFormat>
-      <AdditionalIncludeDirectories>$(SolutionDir)\src\inc;$(SolutionDir)\dep;$(SolutionDir)\dep\Console;$(SolutionDir)\dep\chromium;$(SolutionDir)\dep\Win32K;$(SolutionDir)\dep\gsl\include;$(SolutionDir)\dep\wil\include;%(AdditionalIncludeDirectories);</AdditionalIncludeDirectories>
+      <AdditionalIncludeDirectories>$(SolutionDir)\src\inc;$(SolutionDir)\dep;$(SolutionDir)\dep\Console;$(SolutionDir)\dep\chromium;$(SolutionDir)\dep\dynamic_bitset;$(SolutionDir)\dep\libpopcnt;$(SolutionDir)\dep\Win32K;$(SolutionDir)\dep\gsl\include;$(SolutionDir)\dep\wil\include;%(AdditionalIncludeDirectories);</AdditionalIncludeDirectories>


Will this be fine when we ingest in inbox as well?

@DHowett-MSFT has been fixing these with vpacks on ingestion. We'll probably have to come up with a slightly different strategy though like making a .\dep\noreplicate\ folder and a .\dep\replicate folder and changing the consolegit2gitfilters.json or something since this is happening now and probably more often going forward.

Yeah, Michael's been making my life difficult with all these deps 😉

DHowett-MSFT · 2020-03-24T20:38:48Z

@msftbot merge this in 1 hour

ghost · 2020-03-24T20:38:52Z

Hello @DHowett-MSFT!

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 won't merge this pull request until after the UTC date Tue, 24 Mar 2020 21:38:50 GMT, which is in 1 hour

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".

vadimkantorov · 2020-03-24T22:34:31Z

@DHowett-MSFT (just in case you didn't consider https://github.com/RoaringBitmap/CRoaring which does popcnt stuff internally as well and seems a way more battle-test than https://github.com/pinam45/dynamic_bitset )

Improve wide glyph support in UIA (GH-4946) Add enhanced key support for ConPty (GH-5021) Set DxRenderer non-text alias mode (GH-5149) Reduce CursorChanged Events for Accessibility (GH-5196) Add more object ID tracing for Accessibility (GH-5215) Add SS3 cursor key encoding to ConPty (GH-5383) UIA: Prevent crash from invalid UTR endpoint comparison (GH-5399) Make CodepointWidthDetector::GetWidth faster (CC-3727) add til::math, use it for float conversions to point, size (GH-5150) Add support for renderer backoff, don't FAIL_FAST on 3x failures, add UI (GH-5353) Fix a deadlock and a bounding rects issue in UIA (GH-5385) Don't duplicate spaces from potentially-wrapped EOL-deferred lines (GH-5398) Reimplement the VT tab stop functionality (CC-5173) Clamp parameter values to a maximum of 32767. (CC-5200) Prevent the cursor type being reset when changing the visibility (CC-5251) Make RIS switch back to the main buffer (CC-5248) Add support for the DSR-OS operating status report (CC-5300) Update the virtual bottom location if the cursor moves below it (CC-5317) ci: run spell check in CI, fix remaining issues (CC-4799) (CC-5352) Set Cascadia Code as default font (GH-5121) Show a double width cursor for double width characters (GH-5319) Delegate all character input to the character event handler (CC-4192) Update til::bitmap to use dynamic_bitset<> + libpopcnt (GH-5092) Merged PR 4465022: [Git2Git] Merged PR 4464559: Console: Ingest OSS changes up to e055079 Correct scrolling invalidation region for tmux in pty w/ bitmap (GH-5122) Render row-by-row instead of invalidating entire screen (GH-5185) Make conechokey use ReadConsoleInputW by default (GH-5148) Manually pass mouse wheel messages to TermControls (GH-5131) This fixes C-M-space for WSL but not for Win32, but I'm not sure there's a problem in Win32 quite yet. (GH-5208) Fix copying wrapped lines by implementing better scrolling (GH-5181) Emit lines wrapped due to spaces at the end correctly (GH-5294) Remove unneeded whitespace (CC-5162)

ghost · 2020-05-05T22:27:49Z

🎉Windows Terminal Release Candidate v0.11.1251.0 (1.0rc1) has been released which incorporates this pull request.:tada:

Handy links:

miniksa added 3 commits March 23, 2020 15:47

Import pinam45/dynamic_bitset and optional dependency kimwalisch/libp…

dacbd34

…opcnt. Update NOTICE, include LICENSES, add all governance traceback. Include in common paths and common library imports.

Integrate into bitmap.

1bc35ed

Remove old statement about std::vector<bool>

d8b6263

miniksa requested review from carlos-zamora, DHowett-MSFT, zadjii-msft and leonMSFT March 23, 2020 23:23

miniksa self-assigned this Mar 23, 2020

miniksa changed the title ~~Dev/miniksa/dyn bitset~~ Update til::bitmap to replace std::vector<bool> with dynamic_bitset<> supported by libpopcnt Mar 23, 2020

DHowett-MSFT approved these changes Mar 23, 2020

View reviewed changes

zadjii-msft reviewed Mar 24, 2020

View reviewed changes

SA fix, noexcept. Add includes to universal project path.

e940c18

DHowett-MSFT added the Needs-Second It's a PR that needs another sign-off label Mar 24, 2020

DHowett-MSFT changed the title ~~Update til::bitmap to replace std::vector<bool> with dynamic_bitset<> supported by libpopcnt~~ Update til::bitmap to use dynamic_bitset<> + libpopcnt Mar 24, 2020

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 24, 2020

Merge branch 'master' into dev/miniksa/dyn_bitset

338b608

zadjii-msft approved these changes Mar 25, 2020

View reviewed changes

ghost merged commit 680577f into master Mar 25, 2020

ghost deleted the dev/miniksa/dyn_bitset branch March 25, 2020 02:41

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update til::bitmap to use dynamic_bitset<> + libpopcnt #5092

Update til::bitmap to use dynamic_bitset<> + libpopcnt #5092

miniksa commented Mar 23, 2020 •

edited by DHowett-MSFT

Loading

DHowett-MSFT Mar 23, 2020

miniksa Mar 23, 2020

zadjii-msft left a comment

zadjii-msft Mar 24, 2020

miniksa Mar 24, 2020

DHowett-MSFT Mar 24, 2020

DHowett-MSFT commented Mar 24, 2020

ghost commented Mar 24, 2020

vadimkantorov commented Mar 24, 2020

ghost commented May 5, 2020

Update til::bitmap to use dynamic_bitset<> + libpopcnt #5092

Update til::bitmap to use dynamic_bitset<> + libpopcnt #5092

Conversation

miniksa commented Mar 23, 2020 • edited by DHowett-MSFT Loading

Details std::vector<bool> provided by the Microsoft VC Runtime is

Validation

DHowett-MSFT Mar 23, 2020

Choose a reason for hiding this comment

miniksa Mar 23, 2020

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Mar 24, 2020

Choose a reason for hiding this comment

miniksa Mar 24, 2020

Choose a reason for hiding this comment

DHowett-MSFT Mar 24, 2020

Choose a reason for hiding this comment

DHowett-MSFT commented Mar 24, 2020

ghost commented Mar 24, 2020

vadimkantorov commented Mar 24, 2020

ghost commented May 5, 2020

miniksa commented Mar 23, 2020 •

edited by DHowett-MSFT

Loading

Details `std::vector<bool>` provided by the Microsoft VC Runtime is