Replace StaticStringMap with far more optimized version

[RetroDev256]

Optimized StaticStringMap & Introduction of StaticArrayMap

New Features:

• std.StaticArrayMap and std.StaticArrayMapWithEql have been added. They provide an optimized static mapping from arbitrary slice types to values.
• std.StaticStringMapIgnoreCase has been added. It is a handy case-insensitive static string map.

Changes:

• std.StaticStringMapWithEql: The eql function interface has changed - it takes a comptime-known array length, one comptime known key array, and one runtime-known key array.
• The runtime-known key/value init() function has been removed. Please use initComptime() if possible. For use-cases where key/value pairs may only be known at runtime, std.StringHashMap may be a good alternative to the old behavior.
• The getLongestPrefix and getLongestPrefixIndex functions have been removed. Originally added to see if they would be useful, they did not see much use.

Performance:

Raw StaticStringMap performance for get and has operations has tripled:

Test cases were modified or removed based on whether they were testing the removed runtime-known key/value initialization, getLongestPrefix, or getLongestPrefixIndex.

[nektro]

impressive results! can you post the benchmark(s) source code and also expose a version that lets u8 be generic as well? eg uses []const K

[travisstaloch]

Why remove the initComptime() and init() API? The idea behind #19719 was to move kvs_list from a type param to a init() param in order to avoid very long type names which result from them being a type param.

EDIT: I like the simplicity of this implementation and the results seem to speak for themselves 👍 . FWIW, I don't care about runtime init(). But I think keeping the StaticStringMap name and moving kvs_list to an initComptime() param might be preferrable.

[RetroDev256]

Why remove the initComptime() and init() API? The idea behind #19719 was to move kvs_list from a type param to a init() param in order to avoid very long type names which result from them being a type param.

EDIT: I like the simplicity of this implementation and the results seem to speak for themselves 👍 . FWIW, I don't care about runtime init(). But I think keeping the StaticStringMap name and moving kvs_list to an initComptime() param might be preferrable.

Thank you for explaining the original reasoning! It's well founded. - I'll work on that and see how soon I can get it working.

What are your thoughts on replacing initComptime() with init(), assuming we never use this anymore for runtime kvs?

[RetroDev256]

Seems like the x86_64 backend hasn't implemented gte vector comparison yet.
I'll put a TODO in the code for now, with a simple path for this backend.

[andrewrk]

Impressive results.

Please document the breaking changes in the PR writeup (pretend you are writing release notes).

Please also explain why you deleted a lot of test coverage, otherwise it's difficult to be sure these changes don't break existing use cases.

[RetroDev256]

Please document the breaking changes in the PR writeup (pretend you are writing release notes).

Please also explain why you deleted a lot of test coverage, otherwise it's difficult to be sure these changes don't break existing use cases.

Sounds good - let me know if you would like further changes.

[RetroDev256]

I updated the simple benchmarking code:
Read main.zig for instructions.

bench.zip

[JonathanHallstrom]

i started a review pointing out an issue in toLowerSimd but it doesnt seem to be working so I'll post it here:

    const true_vec: BVec = @splat(true);
    const is_upper: BVec = @select(bool, at_min, at_max, true_vec);

    const lowered: SVec = input + @as(SVec, @splat('a' - 'A'));

this has two issues: is_upper incorrectly identifying [0, 65) as being uppercase letters, and lowered potentially overflowing, which wouldn't be an error, so it should be +%. i propose you change it in the following way:

-    const true_vec: BVec = @splat(true);
-    const is_upper: BVec = @select(bool, at_min, at_max, true_vec);
+    const false_vec: BVec = @splat(false);
+    const is_upper: BVec = @select(bool, at_min, at_max, false_vec);

-    const lowered: SVec = input + @as(SVec, @splat('a' - 'A'));
+    const lowered: SVec = input +% @as(SVec, @splat('a' - 'A'));

comptime code to verify that the changed version works
if you change it to the old implementation of toLowerSimd you will see that it no longer compiles

[RetroDev256]

I understand that this PR removes 3 things -

• runtime initialization of StaticStringMap
• getLongestPrefix
• getLongestPrefixIndex

I am confident that the last two points are useless and wouldn't cause mass community outrage if removed, but I am not entirely sure about the first point. While I couldn't find any uses of StaticStringMap where the key/value pairs weren't comptime, My searching doesn't mean that use-cases don't exist.

I am happy closing this PR if it is deemed that the feature culling is a significant downside. In case it is not though, what tweaks should I make to this PR to improve the quality? Wondering if there is anything that can be done to improve it, or even cut out more unnecessary features.

[squeek502]

• runtime initialization of StaticStringMap

This feature is relatively new, and as I understand it, was mostly a side-effect of other changes in #19719. That is, it wasn't motivated by a particular use case, but was instead just added because it was now possible to provide (correct me if I'm wrong, @travisstaloch).

[travisstaloch]

That is correct. I don't think removing runtime init() is or the getLongestPrefix() functions is going to be a big deal. They were only added as extra, maybe nice to haves. And getLongestPrefix() was doing a linear scan so its probably better to remove it.

[RetroDev256]

I finally learned how to use git, and resolved that pesky rebase conflict, squashing my changes into one commit. If this issue warrants some release notes, the commit message is a good resource. Please let me know if there is something different that should be done for this PR.

[RetroDev256]

I'm pretty sure the failures are unrelated, but I am curious about the aarch64-linux-release failure. Looks like it is incremental related. Is this test nondeterministic by any chance?

[mlugg]

My working theory for our inconsistent CI failures is that some system condition when under load causes processes to frequently crash. That aligns with what we see here -- note that on the failed incremental test, we also get a BrokenPipe on another step, which likely indicates a crash.

It's possible there is an incremental test case which is sometimes crashing the compiler, but I'd have to see the crash on a *-debug CI run to know for sure.

I'll re-run the failed jobs when the in-progress one finishes.

EDIT: oh, that incremental test totally is actually flaky, my bad -- see this run. Will fix now!