From 4f55568a1705a55b48dcb51224ba22f2774fcfa5 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 19 Jun 2020 17:55:17 +0200 Subject: [PATCH] Improved ATTR_ROW::ReplaceAttrs performance (#6573) ## Summary of the Pull Request Improve `ATTR_ROW::ReplaceAttrs` performance by only reserving the necessary capacity instead of resizing the new run. That way `TextAttributeRun`s are only instantiated once instead of twice. ## PR Checklist * [ ] Closes #xxx * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Tests added/passed * [ ] Requires documentation to be updated * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Detailed Description of the Pull Request / Additional comments Performance could be further improved by directly moving `TextAttributeRun`s into the new vector, but I considered this out of scope for this PR. ## Validation Steps Performed CPU usage when running `cacafire` is slightly reduced. --- src/buffer/out/AttrRow.cpp | 67 +++++++++++++------------------------- 1 file changed, 22 insertions(+), 45 deletions(-) diff --git a/src/buffer/out/AttrRow.cpp b/src/buffer/out/AttrRow.cpp index b12a2860a..42f1790b2 100644 --- a/src/buffer/out/AttrRow.cpp +++ b/src/buffer/out/AttrRow.cpp @@ -264,15 +264,16 @@ void ATTR_ROW::ReplaceAttrs(const TextAttribute& toBeReplacedAttr, const TextAtt { // First we try to find the run where the insertion happens, using lowerBound and upperBound to track // where we are currently at. + const auto begin = _list.begin(); size_t lowerBound = 0; size_t upperBound = 0; for (size_t i = 0; i < _list.size(); i++) { - upperBound += _list.at(i).GetLength(); + const auto curr = begin + i; + upperBound += curr->GetLength(); + if (iStart >= lowerBound && iStart < upperBound) { - const auto curr = std::next(_list.begin(), i); - // The run that we try to insert into has the same color as the new one. // e.g. // AAAAABBBBBBBCCC @@ -385,7 +386,7 @@ void ATTR_ROW::ReplaceAttrs(const TextAttribute& toBeReplacedAttr, const TextAtt // fact that an existing piece of the run was split in half (to hold the latter half). const size_t cNewRun = _list.size() + newAttrs.size() + 1; std::vector newRun; - newRun.resize(cNewRun); + newRun.reserve(cNewRun); // We will start analyzing from the beginning of our existing run. // Use some pointers to keep track of where we are in walking through our runs. @@ -393,10 +394,9 @@ void ATTR_ROW::ReplaceAttrs(const TextAttribute& toBeReplacedAttr, const TextAtt // Get the existing run that we'll be updating/manipulating. const auto existingRun = _list.begin(); auto pExistingRunPos = existingRun; - const auto pExistingRunEnd = existingRun + _list.size(); + const auto pExistingRunEnd = _list.end(); auto pInsertRunPos = newAttrs.begin(); size_t cInsertRunRemaining = newAttrs.size(); - auto pNewRunPos = newRun.begin(); size_t iExistingRunCoverage = 0; // Copy the existing run into the new buffer up to the "start index" where the new run will be injected. @@ -410,7 +410,7 @@ void ATTR_ROW::ReplaceAttrs(const TextAttribute& toBeReplacedAttr, const TextAtt iExistingRunCoverage += pExistingRunPos->GetLength(); // Copy it to the new run buffer and advance both pointers. - *pNewRunPos++ = *pExistingRunPos++; + newRun.push_back(*pExistingRunPos++); } // When we get to this point, we've copied full segments from the original existing run @@ -429,12 +429,8 @@ void ATTR_ROW::ReplaceAttrs(const TextAttribute& toBeReplacedAttr, const TextAtt // We need to fix this up below so it says G2 instead to leave room for the Y3 to fit in // the new/final run. - // Copying above advanced the pointer to an empty cell beyond what we copied. - // Back up one cell so we can manipulate the final item we copied from the existing run to the new run. - pNewRunPos--; - // Fetch out the length so we can fix it up based on the below conditions. - size_t length = pNewRunPos->GetLength(); + size_t length = newRun.back().GetLength(); // If we've covered more cells already than the start of the attributes to be inserted... if (iExistingRunCoverage > iStart) @@ -449,7 +445,7 @@ void ATTR_ROW::ReplaceAttrs(const TextAttribute& toBeReplacedAttr, const TextAtt // Now we're still on that "last cell copied" into the new run. // If the color of that existing copied cell matches the color of the first segment // of the run we're about to insert, we can just increment the length to extend the coverage. - if (pNewRunPos->GetAttributes() == pInsertRunPos->GetAttributes()) + if (newRun.back().GetAttributes() == pInsertRunPos->GetAttributes()) { length += pInsertRunPos->GetLength(); @@ -460,18 +456,11 @@ void ATTR_ROW::ReplaceAttrs(const TextAttribute& toBeReplacedAttr, const TextAtt } // We're done manipulating the length. Store it back. - pNewRunPos->SetLength(length); - - // Now that we're done adjusting the last copied item, advance the pointer into a fresh/blank - // part of the new run array. - pNewRunPos++; + newRun.back().SetLength(length); } // Bulk copy the majority (or all, depending on circumstance) of the insert run into the final run buffer. - std::copy_n(pInsertRunPos, cInsertRunRemaining, pNewRunPos); - - // Advance the new run pointer into the position just after everything we copied. - pNewRunPos += cInsertRunRemaining; + std::copy_n(pInsertRunPos, cInsertRunRemaining, std::back_inserter(newRun)); // We're technically done with the insert run now and have 0 remaining, but won't bother updating its pointers // and counts any further because we won't use them. @@ -488,9 +477,6 @@ void ATTR_ROW::ReplaceAttrs(const TextAttribute& toBeReplacedAttr, const TextAtt // If we still have original existing run cells remaining, copy them into the final new run. if (pExistingRunPos != pExistingRunEnd || iExistingRunCoverage != (iEnd + 1)) { - // Back up one cell so we can inspect the most recent item copied into the new run for optimizations. - pNewRunPos--; - // We advanced the existing run pointer and its count to on or past the end of what the insertion run filled in. // If this ended up being past the end of what the insertion run covers, we have to account for the cells after // the insertion run but before the next piece of the original existing run. @@ -514,11 +500,11 @@ void ATTR_ROW::ReplaceAttrs(const TextAttribute& toBeReplacedAttr, const TextAtt // This case is slightly off from the example above. This case is for if the B2 above was actually Y2. // That Y2 from the existing run is the same color as the Y2 we just filled a few columns left in the final run // so we can just adjust the final run's column count instead of adding another segment here. - if (pNewRunPos->GetAttributes() == pExistingRunPos->GetAttributes()) + if (newRun.back().GetAttributes() == pExistingRunPos->GetAttributes()) { - size_t length = pNewRunPos->GetLength(); + size_t length = newRun.back().GetLength(); length += (iExistingRunCoverage - (iEnd + 1)); - pNewRunPos->SetLength(length); + newRun.back().SetLength(length); } else { @@ -526,14 +512,14 @@ void ATTR_ROW::ReplaceAttrs(const TextAttribute& toBeReplacedAttr, const TextAtt // its length for the discrepancy in columns not yet covered by the final/new run. // Move forward to a blank spot in the new run - pNewRunPos++; + newRun.emplace_back(); // Copy the existing run's color information to the new run - pNewRunPos->SetAttributes(pExistingRunPos->GetAttributes()); + newRun.back().SetAttributes(pExistingRunPos->GetAttributes()); // Adjust the length of that copied color to cover only the reduced number of columns needed // now that some have been replaced by the insert run. - pNewRunPos->SetLength(iExistingRunCoverage - (iEnd + 1)); + newRun.back().SetLength(iExistingRunCoverage - (iEnd + 1)); } // Now that we're done recovering a piece of the existing run we skipped, move the pointer forward again. @@ -551,34 +537,25 @@ void ATTR_ROW::ReplaceAttrs(const TextAttribute& toBeReplacedAttr, const TextAtt // New Run desired when done = R3 -> B7 // Existing run pointer is on B2. // We want to merge the 2 from the B2 into the B5 so we get B7. - else if (pNewRunPos->GetAttributes() == pExistingRunPos->GetAttributes()) + else if (newRun.back().GetAttributes() == pExistingRunPos->GetAttributes()) { // Add the value from the existing run into the current new run position. - size_t length = pNewRunPos->GetLength(); + size_t length = newRun.back().GetLength(); length += pExistingRunPos->GetLength(); - pNewRunPos->SetLength(length); + newRun.back().SetLength(length); // Advance the existing run position since we consumed its value and merged it in. pExistingRunPos++; } - // OK. We're done inspecting the most recently copied cell for optimizations. - pNewRunPos++; - // Now bulk copy any segments left in the original existing run if (pExistingRunPos < pExistingRunEnd) { - std::copy_n(pExistingRunPos, (pExistingRunEnd - pExistingRunPos), pNewRunPos); - - // Fix up the end pointer so we know where we are for counting how much of the new run's memory space we used. - pNewRunPos += (pExistingRunEnd - pExistingRunPos); + std::copy_n(pExistingRunPos, (pExistingRunEnd - pExistingRunPos), std::back_inserter(newRun)); } } - // OK, phew. We're done. Now we just need to free the existing run, store the new run in its place, - // and update the count for the correct length of the new run now that we've filled it up. - - newRun.erase(pNewRunPos, newRun.end()); + // OK, phew. We're done. Now we just need to free the existing run and store the new run in its place. _list.swap(newRun); return S_OK;