Repository: microsoft/terminal
File: src/cascadia/TerminalControl/ControlCore.cpp (C++)

Code before:
            _FoundMatchHandlers(*this, *foundResults);
        }
    }

    // Method Description:
    // - Search for a single value in a background
    // Arguments:
    // - search: search object to use to find the next match
    // Return Value:
    // - <none>
    winrt::Windows::Foundation::IAsyncOperation<bool> ControlCore::_SearchOne(::Search& search)
    {
        bool found{ false };
        auto weakThis{ get_weak() };

        co_await winrt::resume_background();
        if (auto control{ weakThis.get() })
        {
            // We don't lock the terminal for the duration of the entire search,
            // since if the terminal was modified the search ID will be updated.
            auto lock = _terminal->LockForWriting();
            found = search.FindNext();
        }

        co_await winrt::resume_foreground(_dispatcher);
        co_return found;
    }

    // Method Description:
    // - Updates the index of the current match according to the direction.
    // The index will remain unchanged (usually -1) if the number of matches is 0
    // Arguments:
    // - goForward: if true, move to the next match, else go to previous
    // Return Value:
    // - <none>
    void SearchState::UpdateIndex(bool goForward)
    {
        if (matches.has_value())
        {
            const int numMatches = ::base::saturated_cast<int>(matches->size());
            if (numMatches > 0)
            {
                if (currentMatchIndex == -1)
                {
                    currentMatchIndex = goForward ? 0 : numMatches - 1;
                }
                else
                {
                    currentMatchIndex = (numMatches + currentMatchIndex + (goForward ? 1 : -1)) % numMatches;
                }
            }

Review comment (performance):
Won't this yield N times back and forth between a background and foreground thread? That's _really_ costly. Yielding threads isn't cheap and each transition costs dozens of microseconds. Each back/forth takes in the order of about 0.1ms on my laptop.

If we search for all occurrences of a common word - let's say "the" which appears roughly 10k times in a 1MB of English - we'd be looking at about 1000ms of CPU time. This is in a stark contrast to a regular boyer moore string search which can go through an entire 120x9001 text buffer (= 1MB English) in roughly 0.5ms. While it's easy to see why this was done, I don't think that this code was written because `Searcher` proved itself to be too slow just yet, right?

Basically: I think we should KISS-ify this code and just run `Searcher` through all results all at once. If we then see any issues arise in the future we could consider adding yielding back in and if we do, we should do it batchwise. I think 1k results per batch would be a reasonable start.

---

BTW this would also go hand in hand with a refactoring of the `Search` class, by removing most of it and replacing it with `std::search` given TextBuffer iterators. This would improve its performance by quite a bit, while also removing a good chunk of code. We can then also add a function that returns all results at once.

Code after:
    void ControlCore::Close()
    {
        if (!_IsClosing())
        {
            _closing = true;

            // Ensure Close() doesn't hang, waiting for MidiAudio to finish playing an hour long song.
            _midiAudio.BeginSkip();

            // Stop accepting new output and state changes before we disconnect everything.
            _connectionOutputEventRevoker.revoke();
            _connectionStateChangedRevoker.revoke();
            _connection.Close();
        }
    }

    void ControlCore::_rendererWarning(const HRESULT hr)
    {
        _RendererWarningHandlers(*this, winrt::make<RendererWarningArgs>(hr));
    }

    winrt::fire_and_forget ControlCore::_renderEngineSwapChainChanged(const HANDLE sourceHandle)
    {
        // `sourceHandle` is a weak ref to a HANDLE that's ultimately owned by the
        // render engine's own unique_handle. We'll add another ref to it here.
        // This will make sure that we always have a valid HANDLE to give to
        // callers of our own SwapChainHandle method, even if the renderer is
        // currently in the process of discarding this value and creating a new
        // one. Callers should have already set up the SwapChainChanged
        // callback, so this all works out.

        winrt::handle duplicatedHandle;
        const auto processHandle = GetCurrentProcess();
        THROW_IF_WIN32_BOOL_FALSE(DuplicateHandle(processHandle, sourceHandle, processHandle, duplicatedHandle.put(), 0, FALSE, DUPLICATE_SAME_ACCESS));

        const auto weakThis{ get_weak() };

        // Concurrent read of _dispatcher is safe, because Detach() calls WaitForPaintCompletionAndDisable()
        // which blocks until this call returns. _dispatcher will only be changed afterwards.
        co_await wil::resume_foreground(_dispatcher);

        if (auto core{ weakThis.get() })
        {
            // `this` is safe to use now

            _lastSwapChainHandle = std::move(duplicatedHandle);
            // Now bubble the event up to the control.
            _SwapChainChangedHandlers(*this, winrt::box_value<uint64_t>(reinterpret_cast<uint64_t>(_lastSwapChainHandle.get())));
        }
    }