New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve Base64::Decode performance #11467

Merged

8 commits merged into main from dev/lhecker/base64-perf

Oct 26, 2021

Member

lhecker commented Oct 8, 2021 •

edited

Loading

This commit renames Base64::s_Decode into Base64::Decode and improves its
average performance on short strings of less than 200 characters by 4.5x.
This is achieved by implementing a classic base64 decoder that reads 4
characters at a time and produces 3 output bytes. Furthermore a small
128 byte lookup table is used to quickly map characters to values.

PR Checklist

I work here
Tests added/passed

Validation Steps Performed

Run WSL in Windows Terminal
Run printf "\033]52;c;aHR0cHM6Ly9naXRodWIuY29tL21pY3Jvc29mdC90ZXJtaW5hbC9wdWxsLzExNDY3\a"
Clipboard contains https://github.com/microsoft/terminal/pull/11467 ✔️

lhecker force-pushed the dev/lhecker/base64-perf branch 3 times, most recently from b91b899 to 91d81d4 Compare

October 8, 2021 22:15

lhecker commented

View reviewed changes

src/terminal/parser/OutputStateMachineEngine.cpp Outdated Show resolved Hide resolved

src/terminal/parser/ut_parser/Base64Test.cpp

-                      VERIFY_ARE_EQUAL(true, success);
-                      VERIFY_ARE_EQUAL(L"foo", result);
+                      // NOTE: Modify testRounds to get the feeling of running a fuzz test on Base64::Decode.
+                      static constexpr auto testRounds = 8;

Member Author

lhecker Oct 8, 2021

I've run this test 10 times with 1M rounds each.
It tests the decode function with valid base64 strings of random length between 0 and 128 bytes, with and without trailing =.

What's missing are tests for invalid base64. However given the pointer arithmetic and lack of if conditions based on input in my loop I'm confident that it behaves correctly in such situations.


          Improve Base64::Decode performance

273f870

lhecker force-pushed the dev/lhecker/base64-perf branch from 91d81d4 to 273f870 Compare

October 8, 2021 23:05

lhecker commented

View reviewed changes

src/terminal/parser/ut_parser/OutputEngineTest.cpp

@@ @@ -3255,7 +3255,7 @@ class StateMachineExternalTest final @@
                       pDispatch->_copyContent = L"UNCHANGED";
                       // Passing a non-base64 `Pd` param is illegal, won't change the content.
-                      mach.ProcessString(L"\x1b]52;;foo\x07");
+                      mach.ProcessString(L"\x1b]52;;???\x07");

Member Author

lhecker Oct 8, 2021

"foo" is actually a valid base64 string according to RFC 4648.

DHowett approved these changes

View reviewed changes

Member

DHowett left a comment

Clever.

.github/actions/spelling/expect/expect.txt Outdated

@@ @@ -228,6 +228,8 @@ cfae @@
               Cfg
               cfie
               cfiex
+              Cfj

Member

DHowett Oct 12, 2021

interesting -- I wonder if we should not add base64 snippets into the expect list and instead pattern or file exclude them?

src/terminal/parser/OutputStateMachineEngine.cpp Outdated Show resolved Hide resolved

src/terminal/parser/base64.cpp Outdated Show resolved Hide resolved

src/terminal/parser/base64.cpp Outdated

+                  do                                         \
+                  {                                          \
+                      const auto n = decodeTable[ch & 0x7f]; \
+                      error |= (ch | n) & 0xff80;            \

Member

DHowett Oct 12, 2021

wat

how does this work?

Member

DHowett Oct 12, 2021

okay so error is effectively, "is the loaded value 255 (the invalid sentinel in the table) or the character larger than 7 bits (clearly invalid ASCII)"

Member Author

lhecker Oct 12, 2021

I added some comments explaining this.

src/terminal/parser/base64.cpp

                       }
-                      switch (state)
+                      switch (ri)

Member

DHowett Oct 12, 2021

ri => remainder index?

Member Author

lhecker Oct 12, 2021

I think I called r because it's the accumulation "register" (since on a CPU level r will almost certainly live inside a register most of the time).
And since i is usually a counter for something I called it ri, the "register index-counter-thingy".

"remainder index" however is much better. I'll steal that. 😄

lhecker added 4 commits

October 12, 2021 18:12


          Merge remote-tracking branch 'origin/main' into dev/lhecker/base64-perf

1b4181c


          Improve documentation

da3406d


          Address spelling linter feedback

e59eb93


          Replace exceptions with HRESULTs

60312dd

This comment has been minimized.

Sign in to view

lhecker added 3 commits

October 13, 2021 01:51


          Fix spelling

c27da5e


          Fix _GetOscSetClipboard return value

9c8dc32


          Fix audit checks

6be7ce6

DHowett reviewed

View reviewed changes

src/terminal/parser/base64.cpp

+              // Decodes an UTF8 string encoded with RFC 4648 (Base64) and returns it as UTF16 in dst.
+              // It supports both variants of the RFC (base64 and base64url), but
+              // throws an error for non-alphabet characters, including newlines.
+              // * Throws an exception for all invalid base64 inputs.

Member

DHowett Oct 13, 2021

well,

DHowett reviewed

View reviewed changes

src/terminal/parser/base64.cpp

+              // It supports both variants of the RFC (base64 and base64url), but
+              // throws an error for non-alphabet characters, including newlines.
+              // * Throws an exception for all invalid base64 inputs.
+              // * Doesn't support whitespace and will throw an exception for such strings.

Member

DHowett Oct 13, 2021

i bet that this will come bite us later, but i am willing to take that risk

Member Author

lhecker Oct 13, 2021

Oh it’s "return ERROR_INVALID_DATA" now. I‘ll update the comment.
Is there anything else you’re worried about?

DHowett approved these changes

View reviewed changes

DHowett added the Needs-Second label

ghost requested review from zadjii-msft, miniksa, carlos-zamora, leonMSFT and PankajBhojwani

October 15, 2021 23:08

miniksa reviewed

View reviewed changes

src/terminal/parser/ut_parser/Base64Test.cpp

Comment on lines +43 to +46

    
                          for (auto& i : randomData)

                          {

                              i = rng();

                          }

Member

miniksa Oct 22, 2021

This almost felt like a std::generate moment.

Member Author

lhecker Oct 22, 2021

I could‘ve used std::generate but that would force me to add calls to std::begin, std::end and std::ref as well just to generate worse assembly (which really doesn’t matter here).
So I left it as a simple loop which I found to be much more readable. 🙂

Member

miniksa Oct 22, 2021

Ah you're right. But it's fun to dream.

miniksa approved these changes

View reviewed changes

zadjii-msft approved these changes

View reviewed changes

zadjii-msft added the AutoMerge label

ghost commented Oct 26, 2021

Hello @zadjii-msft!

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 merged commit 9aa4a11 into main

ghost deleted the dev/lhecker/base64-perf branch

October 26, 2021 21:30

ghost commented Feb 3, 2022

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

Handy links:

This pull request was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

miniksa miniksa approved these changes

zadjii-msft zadjii-msft approved these changes

DHowett DHowett approved these changes

carlos-zamora Awaiting requested review from carlos-zamora

leonMSFT Awaiting requested review from leonMSFT

PankajBhojwani Awaiting requested review from PankajBhojwani

Labels

AutoMerge Needs-Second