Optimize StaticStringMap Implementation

[RetroDev256]

Optimized StaticStringMap

StaticStringMap is an optimized mapping between some key/value pairs. The keys are always strings, and the values can be any type. The structure is initialized with the key/value pairs, and there is no way to change key/value pairs through the lifetime of the type.

This PR replaces the implementation of StaticStringMap.

Runtime init() has been removed. Use initComptime() instead.
Functions getLongestPrefix() and getLongestPrefixIndex() have been removed.
Type StaticStringMapWithEql is no longer pub.
Type StaticStringMapIgnoreCaseAscii has been added. It is a case-insensitive StaticStringMap.
StaticStringMap now checks for redundant keys.
Test cases were removed if they referenced removed features.

The motivation behind this PR is a simplified implementation and improved code generation.
Here is the code generation: https://godbolt.org/z/ojW841qzh
The performance of StaticStringMap has more than doubled for the LLVM backend:

bench.zip

x86_64 backend benchmark for `get` and `has`

Zig built with LLVM running `zig fmt CodeGen.zig`

Zig built with x86_64 backend running `zig fmt CodeGen.zig`

Zig built with LLVM compiling "hello world"

Zig built with x86_64 backend compiling "hello world"

Zig built with LLVM running "zig ast-check CodeGen.zig"

Zig built with x86_64 backend running "zig ast-check CodeGen.zig"

Size difference in Zig compiled with LLVM backend

Size difference in Zig compiled with x86_64 backend

Building the benchmark of both implementations

Closes #20055

[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?

[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.

[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

[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'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!

[alexrp]

test
+- test-modules
   +- test-behavior
      +- run test behavior-riscv64-linux.4.19...6.11.5-musl.2.28-baseline_rv64-Debug-selfhosted-no-lld
         +- zig test Debug riscv64-linux-musl 1 errors
/home/ci/actions-runner4/_work/zig/zig/lib/std/static_array_map.zig:27:5: error: TODO: airBitCast [3]u8 to u24
pub fn defaultEql(comptime expected: anytype, actual: anytype) bool {

@Rexicon226 👀

[RetroDev256]

There is a limitation to both the old approach, and the new approach. Custom equal functions cannot say that two different-length arrays/strings are equal, because in both implementations the length is compared before they even reach the equal function.

There are a few options we can take:

1. Remove all "custom equal function" abstractions, and only provide "all bits match", or (for strings only), "case-insensitive string matching". With this option, we could also consider whether we even want StaticArrayMap, or if StaticStringMap is more than enough for the standard library. (StaticArrayMap is a YAGNI for most use cases).
2. Modify the "WithEql" types to not pre-filter the input strings based on length. The upside is that we can now make custom equal functions that handle strings with different lengths. The downside is that comparing the strings takes longer, and the standard library is made more complicated.
3. Narrow the definition of the custom eql function to say that it is meant to compare individual elements instead of the string as a whole.

The third option could be done in two ways: we could say "make a function that takes one comptime-known element and one runtime-known element, and it decides how to compare them". This has the distinct advantage that the interface is further simplified, but cripples the current advantage of the defaultEql function. The second way to accept the third option is to keep the current interface, changing nothing.

Personally I would prefer the first option, as I like simplicity. In my opinion, we don't need StaticArrayMap, but a comptime-optimized mapping between strings (normal or case-insensitive) is more than enough.

Thoughts?

I could make this into an issue if it's desired, but I figure it would be smoother to make such a change now rather than later.

[andrewrk]

Personally I would prefer the first option, as I like simplicity. In my opinion, we don't need StaticArrayMap, but a comptime-optimized mapping between strings (normal or case-insensitive) is more than enough.

I agree.

[andrewrk]

Please rebase against latest master branch and force push. When I do that with github ui I receive this:

Response.json: Body has already been consumed.

[RetroDev256]

I am interested in getting this landed for the 0.14.0 release, and would be happy to make any adjustments to facilitate this. Is there anything more that may be needed of me?

[andrewrk]

it's a very messy writeup, comment history, commit history, which means it will require careful review, which means it will not have time to be merged before 0.14.0