Add multi-threaded collections

[dunhor]

This mainly re-uses the existing collections, but adds support for doing shared/exclusive operations. For the existing single threaded collections, nothing special is done and operations are performed without any synchronization. For the new multi-threaded collections, an SRWLOCK is acquired in either shared or exclusive mode.

The most noteworthy "flaw" is that mutating iterator operations lock the collection exclusively. This is mostly a limitation imposed by the base IIterable type which uses opaque iterators for the IIterator objects. In theory, the collection only needs to be locked in shared mode, however that would require the IIterator objects themselves to have a separate lock, which seemed like overkill and unlikely to be worth it. Compare this to the existing ABI collection types which know the underlying vector/map types and can perform interlocked operations - in the case of vector - or forgo sequential consistency while still guaranteeing safety - in the case of map.

The following were also discussed, but are not addressed here as they can be done separately as a later change:

• The vector/map types still implement IVectorView/IMapView and GetView returns a pointer to itself, meaning that you can still QI an IVectorView -> IVector
• The IObservable* types still allow modification during a change callout. This can pretty easily be addressed by an atomic_bool as locks aren't really helpful here anyway.

[jonwis]

Looks generally good; is it fair to say that the threadedness is about "this operation increments the version" vs. "this one does not"?

[DefaultRyan]

We seem inconsistent about the requirements we place on the container. Here, we only assume that it has begin() and end(), but make no assumptions about size(). But vector_base assumes that size() exists. I wonder why?

[dunhor]

🤷‍♂️

[kennykerr]

Views can be constructed from iterator pairs and there's no size on winrt::impl::range_container.

[DefaultRyan]

Makes sense. On a side note, I've been toying with the thought of introducing C++20 concepts to C++/WinRT in few places. The collection-related code, in particular, is an area that would benefit.

[kennykerr]

Yes, there are a few C++20 features I'd like to adopt including span and coroutines #676 - we should think about forking cppwinrt with a servicing C++17 branch and start work on C++20 features in master. Just a little nervous about how long it will take for C++20 to become mainstream since Visual C++ doesn't even have a /std:c++20 flag yet.

[DefaultRyan]

Agreed. At least with span, concepts, char8_t, consteval, chrono updates, and otheres, there are clear-cut feature test macros. Coroutine support has the potential to be messier.

[DefaultRyan]

One minor nitpicky suggestion on the map writing test, but looks good.

[oldnewthing]

This destructs the previous value while inside the lock. Since the object being destructed can be an app-controlled object, there is danger of reentrancy and therefore deadlock. (We have found actual product bugs that were traced back to this category of deadlock.)

Need to swap the previous item into an object that exists outside the lock, so that it destructs outside the lock.

auto wrapped = static_cast<D const&>(*this).wrap_value(value);
static_cast<D&>(*this).perform_exclusive([&]
{
   if (index >= ...) { ... }
   this->increment_version();
   swap(static_cast<D&>(*this).get_container()[index], wrapped);
  });
  // wrapped destructs here, outside the lock
}

This needs to be done only for reference types. Value types do not contain app-controlled objects (the only object with a nontrivial destructor that can be in a value type is hstring, and the app doesn't own the hstring destructor.)

In theory, even AddRef is app-controlled, but apps are unlikely to do anything fancy in their AddRef. Doing fancy stuff in the destructor is far more common.

[dunhor]

Good catch

[dunhor]

The latest iteration is a first stab at addressing this. I'm mostly looking for feedback/input on the approach, though it is fully functional. The most important thing is probably delaying destruction of the "wrapped" value vs. delaying destruction of the "unwrapped" value. Of course, delaying destruction of the "wrapped" value implies that the destruction of the "unwrapped"/"underlying" value is also delayed. The approach here is to delay destruction of the "wrapped" value. This seemed to be the safest approach, but is not without its pros/cons.

• This covers the scenario where the "wrapped" type's destructor does something potentially unsafe. I'm unsure how realistic this scenario is, however I figured it's best to be safe.
• We can make fewer assumptions about the "wrapped" type, at least compared to WinRT types. In particular how/if it's constructible. The approach here is to use std::optional so that we can delay (move) construct the value, and that is the only requirement on the type put in place. If the type is not move constructible, we fall back to running the destructor under the lock (a con, but unlikely to ever be a problem). There's also a potential issue if the moved-from value's destructor does something unsafe, however that's pretty much unavoidable no matter what we do.
• As-is, we assume that anything that's not trivially destructible needs to be destructed outside of the lock. This is a bit more restrictive than necessary. E.g. I would assume that hstring should be safe to destroy under the lock (not necessarily a con; doing less under the lock is generally good). Similarly, I would assume that structs with IReference members should be safe to destroy under the lock (but I'm not 100% sure, correct me if I'm wrong).
• This doesn't take into account the "threadedness" of the collection. I.e. the single threaded collections still try and order destruction. This is mostly noteworthy for operations like Clear and ReplaceAll where we may otherwise be able to re-use the buffer. This is mostly a simplicity decision.

Mostly just wanted to get this out to get thoughts/feedback.

[oldnewthing]

I never did figure out what Split does.

[kennykerr]

It does nothing. In theory it was meant to split the container for parallel search algorithms. That wasn't a well thought out plan. Fortunately a "valid" implementation can do nothing.

[kennykerr]

Thanks!