Remove throwing/originating errors in expected scenarios (Lookup/TryLookup and Cancel)

[antmor]

Fixes: #1511 #1376

TL;DR:

We are removing RoOriginate from the default winrt implementations of Map/MapView::Lookup, which improves TryLookup performance in the case of a key missing from the Map/View.
We are also adding a way to elide RoOriginate from cancellation calls, for cppwinrt implementors who use cancellation in non-exceptional circumstances.

Problem Statement:

The winrt::IMapView projection exposes Lookup (which throws on not found), and a TryLookup function (that returns an optional/null) as an optimization to avoid the throw. Both of these projections call ABI::IMap/View::Lookup, which is expected to return an HRESULT, which the projection either throws, or compares to E_BOUNDS and returns nullopt/nullptr. This is an optimization to avoid the possible RPC of doing both a HasKey and a Lookup.

The issue is that the winrt producer of IMapView, does not have such an optimization. it exposes the ABI::IMap/View as winrt::impl::map_impl, which wraps a winrt::IMap/View implementation. The default Map/View implementation is winrt::map_view_base. Therefore when map_impl forwards the Lookup call to winrt::map_view_base::Lookup, is forced to throw an hresult_error, which the winrt::impl::map_impl, will catch, then convert to an HRESULT. This throw/catch is pessimistic, as it's an extra throw, which is a ~6000ns perf hit, and a RoOriginate, which is a ~15000ns hit, and affects performance of Lookup for all calls where Lookup fails. Thus it causes bad performance AND a bad developer/debugging experience due to the extraneous printfs from the RoOriginate.

Similarly, when an async task is cancelled, the await_adapter is expected to throw, but the origination causes that same 15000ns perf hit, and an extra debug output while debugging.

Both of these cases are extraneous use of RoOriginate. Origination is supposed to inform the developer about unexpected scenarios such as misconfigured apps, or unexpected state, possibly with a string telling the developer what went wrong and how to fix it. A missing value in a Map is not a good use of RoOriginate, as there is no helpful string to add to the fact that a map didn't have the value, and the line number isn't helpful either, as the map_view_base is a common implementation for any cppwinrt app that uses the default map.

Solution

The solution is to remove the throw/originate in both Lookup and TryLookup. We create an escape hatch in the map_impl ABI producer, TryLookup, for implementers of IMap/View, such as map_view_base, to use.

• enlighten winrt::impl::map_impl to know about TryLookup and call it if it is present
• implmement a TryLookup function in the winrt::map_view_base
• and make this available for custom IMap implementations to also opt-in to via trylookup_from_abi

for the cancellation optimization

• create an winrt::hresult override that can throw, but not rooriginate
• enable cancellation_token to choose whether to create a winrt::hresult_cancelled with OR without an origination

general improvements

• added a .runsettings file to add the ability to run the tests from Visual Studio (using Catch2ToVisualStudioTest adapter visual studio extension)

Verified:
output of the generation:

    struct produce<D, winrt::Windows::Foundation::Collections::IMapView<K, V>> : produce_base<D, winrt::Windows::Foundation::Collections::IMapView<K, V>>
    {
        int32_t __stdcall Lookup(arg_in<K> key, arg_out<V> winrt_impl_result) noexcept final try
        {
            clear_abi(winrt_impl_result);
            typename D::abi_guard guard(this->shim());
            if constexpr (has_TryLookup_v<D, K>)
            {
                auto out_param_val = this->shim().TryLookup(*reinterpret_cast<K const*>(&key), trylookup_from_abi);
                if (out_param_val) 
                {
                    *winrt_impl_result = detach_from<V>(std::move(*out_param_val));
                }
                else 
                {
                    return impl::error_out_of_bounds; 
                }

            }
            else
            {
                *winrt_impl_result = detach_from<V>(this->shim().Lookup(*reinterpret_cast<K const*>(&key)));
            }
            return 0;
        }
        catch (...) { return to_hresult(); }

ran build_test_all.cmd, and ran tests in Visual Studio:

Time Elapsed 00:01:12.99 =============================================================================== All tests passed (2964 assertions in 94 test cases) =============================================================================== All tests passed (55 assertions in 17 test cases) =============================================================================== All tests passed (9 assertions in 1 test case) =============================================================================== All tests passed (15 assertions in 3 test cases) =============================================================================== All tests passed (11 assertions in 2 test cases) =============================================================================== All tests passed (4765 assertions in 393 test cases) =============================================================================== All tests passed (3 assertions in 1 test case) =============================================================================== All tests passed (7 assertions in 1 test case)

In a simple perf test (done in the scratch app):

void RunValueSetBenchmark(const winrt::Windows::Foundation::Collections::IMap<winrt::hstring, winrt::Windows::Foundation::IInspectable>& map)
{
std::cout << "TryLookup fails" << std::endl;
auto start = winrt::clock::now();
for (int i = 0; i < count; ++i)
{
    [[maybe_unused]] auto v = map.TryLookup(L"missing");
}
auto end = winrt::clock::now();
std::cout << (end - start) / count << " per iteration" << std::endl;

std::cout << "TryLookup succeeds" << std::endl;
start = get_start_time();

for (int i = 0; i < count; ++i)
{
    auto v = map.TryLookup(L"100");
    //FAIL_FAST_IF(!v);
}
end = winrt::clock::now();
std::cout << (end - start) / count << " per iteration" << std::endl;
}

	before	non-originating exception	after
CppWinRT single_threaded_map TryLookup fails	21970ns per iteration	6486ns per iteration	106ns per iteration
TryLookup succeeds	80ns per iteration	80ns per iteration	83ns per iteration
CppWinrt single_threaded_map with unordered_map implemenation TryLookup fails	22506ns per iteration	5231ns per iteration	17ns per iteration
TryLookup succeeds	30ns per iteration	26ns per iteration	27ns per iteration

[jonwis]

Super interesting!

[oldnewthing]

The PR title and description are extremely misleading. It's not “Avoid throwing/originating errors in expected scenarios” because straight Lookup failure is not an expected scenario.

Should be “Remove RoOriginate from Lookup failure (usually expected)”.

The PR description is too focused on TryLookup, which be a red herring for people investigating a Lookup regression.

Should be "We are removing RoOriginate from the internal implementation of Lookup, which benefits TryLookup performance" and then describe how you're doing it.

[oldnewthing]

I'm not convinced that we need this. Can we just call TryLookup if it is present and returns std::optional<T>?

[antmor]

This was added at Ryan's suggestion, to make sure that we don't inadvertently create a new compiler error for possible custom IMap implementations that have their own TryLookup implementations that is incompatible with our usage.

[ChrisGuzak]

seems very unlikely right?

[antmor]

i agreed with his suggestion as since is not a "breaking" change, any code that compiled before this change, should compile after this change. So if a custom IMap has TryLookup(key, extra args) or bool TryLookup(key, outval), we won't break compilation of their code by merely updating their cppwinrt versions.

[oldnewthing]

Shouldn't we add this to all of the hresult_* types? I don't know if slim_source_location works with this consolidation

struct hresult_blah : hresult_error
{
    template<typename...Args>
    hresult_blah(Args&&...args) : hresult_error(impl::error_blah, std::forward<Args>(args)...) {}
};

[antmor]

added test, and would that be a greater change that i would want to make in this PR.

[antmor]

i think it makes sense that if there's no origination, there's no logging. so i removed the error. As far as the greater consolidation, does that make sense to make that change in this PR? would it be too big a diff for this change?

[ChrisGuzak]

seems very unlikely right?

[github-actions]

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[DefaultRyan]

I'm trying to decipher if there is also a test case that verifies that a map with a TryLookup lacking trylookup_from_abi_t does not have its TryLookup called.