Skip to content
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.

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

Introduce til::linear_flat_set #15089

Merged
merged 7 commits into from
Apr 4, 2023
Merged

Introduce til::linear_flat_set #15089

merged 7 commits into from
Apr 4, 2023

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Apr 3, 2023

til::linear_flat_set is a primitive hash map with linear probing.
The implementation is slightly complicated due to the use of templates.
I've strongly considered just writing multiple copies of this class,
by hand since the code is indeed fairly trivial but ended up deciding
against it, because this templated approach makes testing easier.

This class is in the order of 10x faster than std::unordered_map.

@lhecker lhecker added the Area-Quality Stability, Performance, Etc. label Apr 3, 2023
@github-actions

This comment has been minimized.

@lhecker
Copy link
Member Author

lhecker commented Apr 3, 2023

I think the spelling bot isn't correctly segmenting <Type> and </Type> in tools/ConsoleTypes.natvis. It seems to split it into ype and marks that as an incorrect word. 🤔

@github-actions

This comment has been minimized.

@lhecker lhecker force-pushed the dev/lhecker/til-flat-set branch from c536a1c to 567550a Compare April 3, 2023 15:01
src/inc/til/flat_set.h Outdated Show resolved Hide resolved
@zadjii-msft
Copy link
Member

I didn't manage to read all this yet but a unittest might help clarify how this is supposed to be used (even if it's just as a map)

@github-actions

This comment has been minimized.

@lhecker
Copy link
Member Author

lhecker commented Apr 4, 2023

I've simplified the code a lot and it should be way easier to review now. 🙂

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love it, but I don't totally get why you got rid of the (concept??) thing that probably made better error messages if you tried to make a lfs with a type that didn't support it?

@lhecker
Copy link
Member Author

lhecker commented Apr 4, 2023

I felt like it made understanding the flow of code unnecessarily complex and required you to understand all parts of the code at once. For instance, this is a realistic implementation of the trait:

namespace til
{
    template<>
    struct flat_set_trait<BackendD3D::AtlasFontFaceEntry>
    {
        using T = BackendD3D::AtlasFontFaceEntry;

        static size_t hash(const BackendD3D::AtlasFontFaceKey& key) noexcept
        {
            return flat_set_hash_integer(std::bit_cast<uintptr_t>(key.fontFace) | static_cast<u8>(key.lineRendition));
        }

        static size_t hash(const T& slot) noexcept
        {
            const auto& inner = *slot.inner;
            return flat_set_hash_integer(std::bit_cast<uintptr_t>(inner.fontFace.get()) | static_cast<u8>(inner.lineRendition));
        }

        static bool equals(const T& slot, const BackendD3D::AtlasFontFaceKey& key) noexcept
        {
            const auto& inner = *slot.inner;
            return inner.fontFace.get() == key.fontFace && inner.lineRendition == key.lineRendition;
        }

        static bool empty(const T& slot) noexcept
        {
            return !slot.inner;
        }

        static void fill(T& slot, const BackendD3D::AtlasFontFaceKey& key)
        {
            slot.inner = std::make_unique<BackendD3D::AtlasFontFaceEntryInner>();

            auto& inner = *slot.inner;
            inner.fontFace = key.fontFace;
            inner.lineRendition = key.lineRendition;
        }

        static std::unique_ptr<T[]> allocate(size_t capacity)
        {
            return std::make_unique<T[]>(capacity);
        }

        static void clear(T* data, size_t capacity) noexcept
        {
            for (auto& slot : std::span{ data, capacity })
            {
                slot.inner.reset();
            }
        }
    };
}

The definition of the linear_flat_set is in a different file, just like the definition of the AtlasFontFaceEntry struct itself. So, to understand how everything fits together you had to have 3 files open at once. The new approach is simpler in a way, because the 4 functions you need to implement now are all "standard" C++ functions: A hash function, bool operator, comparison and copy-assignment operator. Most types already implement these by default. Technically I could even use std::hash instead of a hash member function and there would only be standard C++ left.

@DHowett
Copy link
Member

DHowett commented Apr 4, 2023

Got it.

Well, do you think that we would benefit from a concept that guides an implementer in, at least, the right direction?

@lhecker
Copy link
Member Author

lhecker commented Apr 4, 2023

I can't quite figure out how to do that. I tried:

template<typename U>
std::pair<T&, bool> insert(U&& key)
    requires Hashable<U> && std::equality_comparable_with<T, U> && std::assignable_from<T, U>

but it failed to compile. I'm not sure what's wrong with that, but tbh, that concept code didn't even result in a particularly easy to understand message on MSVC in the first place. It said:

1>src\til\ut_til\FlatSetTests.cpp(59,45): error C2672: 'til::linear_flat_set<Data,2>::insert': no matching overloaded function found
1>src\inc\til\flat_set.h(56,29): message : could be 'std::pair<T &,bool> til::linear_flat_set<T,2>::insert(U &&)'
1>        with
1>        [
1>            T=Data
1>        ]
1>src\til\ut_til\FlatSetTests.cpp(59,45): message : the associated constraints are not satisfied
1>src\inc\til\flat_set.h(57,22): message : the concept 'std::assignable_from<Data,int>' evaluated to false
1>C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\include\concepts(93,27): message : the constraint was not satisfied
1>src\til\ut_til\FlatSetTests.cpp(59,42): error C2119: '<structured binding>': the type for 'const auto' cannot be deduced from an empty initializer
1>src\til\ut_til\FlatSetTests.cpp(59,42): error C3617: initializers of structured bindings must be of array or non-union class type; type 'int' is not permitted

@DHowett
Copy link
Member

DHowett commented Apr 4, 2023

eh, screw it then :)

@lhecker lhecker merged commit 9dfdf2a into main Apr 4, 2023
@lhecker lhecker deleted the dev/lhecker/til-flat-set branch April 4, 2023 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Quality Stability, Performance, Etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants