ComptimeStringMap: return a regular struct and optimize #19719

travisstaloch · 2024-04-21T06:47:00Z

this patch renames ComptimeStringMap to StaticStringMap, makes it
accept only a single type parameter, and return a known struct type
instead of an anonymous struct. initial motivation for these changes
was to reduce the 'very long type names' issue described here
#19682.

this breaks the previous API. users will now need to write:
const map = std.StaticStringMap(T).initComptime(kvs_list);

move kvs_list param from type param to an initComptime() param
new public methods
- keys(), values() helpers
- init(allocator), deinit(allocator) for runtime data
- getLongestPrefix(str), getLongestPrefixIndex(str) - i'm not sure
  these belong but have left in for now incase they are deemed useful
performance notes:
- i posted some benchmarking results here:
  travisstaloch/comptime-string-map-revised#1
- i noticed a speedup reducing the size of the struct from 48 to 32
  bytes and thus use u32s instead of usize for all length fields
- i noticed speedup storing KVs as a struct of arrays
- latest benchmark shows these wall_time improvements for
  debug/safe/small/fast builds: -6.6% / -10.2% / -19.1% / -8.9%. full
  output in link above.

Vexu · 2024-04-21T06:58:13Z

Could the name be changed while we're breaking the API? Instead of ComptimeStringMap with initRuntime have something like StaticStringMap with initComptime?

travisstaloch · 2024-04-21T06:59:37Z

sure no problem. will work on that now.

travisstaloch · 2024-04-21T07:49:42Z

should I leave in initRuntime()? if so, should I rename it to init() now?

travisstaloch · 2024-04-21T08:13:50Z

Could the name be changed while we're breaking the API? Instead of ComptimeStringMap with initRuntime have something like StaticStringMap with initComptime?

This is done. We'll see how many typos i made 😄

travisstaloch · 2024-04-21T08:15:52Z

CI doesn't like that I've renamed lib/std/comptime_string_map.zig to lib/std/static_string_map.zig

ninja: error: '../lib/std/comptime_string_map.zig', needed by 'zig2.c', missing and no known rule to make it

travisstaloch · 2024-04-21T08:25:51Z

CI doesn't like that I've renamed lib/std/comptime_string_map.zig to lib/std/static_string_map.zig

For now, I've moved back to the original name so we can make progress.

BratishkaErik · 2024-04-21T08:45:49Z

CI doesn't like that I've renamed lib/std/comptime_string_map.zig to lib/std/static_string_map.zig
ninja: error: '../lib/std/comptime_string_map.zig', needed by 'zig2.c', missing and no known rule to make it

I think you need to also change this line (in samencommit), so that CMake knows this "dependency" is renamed:

zig/CMakeLists.txt

Line 225 in 3c5e840

"${CMAKE_SOURCE_DIR}/lib/std/comptime_string_map.zig"

travisstaloch · 2024-04-21T08:50:31Z

I think you need to also change this line (in samencommit), so that CMake knows this "dependency" is renamed:

Thanks! Just pushed this change.

Vexu · 2024-04-21T14:17:37Z

if so, should I rename it to init() now?

That's what I was going for.

travisstaloch · 2024-04-21T14:32:53Z

Sounds good! Thats an easy change.

Also, any thoughts about whether I should remove getPartial()? I found it very useful for tokenizing or parsing to have a way to check if a string starts with any key from the map and thats why I've left it in. If it should stay, any idea of a better name than getPartial(str)? Something like getLongestKVAtStartOf(str)? I'm struggling with the name.

erikarvstedt · 2024-04-21T14:49:57Z

getMaxPrefix

/// Returns the key-value pair for the longest key that is a prefix of str.

travisstaloch · 2024-04-21T14:56:05Z

getMaxPrefix

Not bad. I think I'd prefer getLongestPrefix() 🤔

travisstaloch · 2024-04-21T21:55:03Z

That's what I was going for.

@Vexu done - renamed initRuntime() to init(). Also renamed getPartial() to getLongestPrefix() and cleaned up tests and documentation a little more. I removed most of the duplicated testing of runtime init(), leaving only a couple. So the tests look a lot more like they did before this PR.

travisstaloch · 2024-04-21T23:20:47Z

Perf point: building my simdjzon project with around 7000 loc zig files in src folder.

Note that I'm not super confident in my process so here are some notes:

zig below is a release build i built yesterday (../build-release/stage3/bin/zig) and is available in $PATH.
../zig/stage4/bin/zig was built with ../build-release/stage3/bin/zig build -p stage4 -Denable-llvm -Dno-lib -Doptimize=ReleaseFast.

.../zig/simdjzon $ zig version
0.13.0-dev.8+c352845e8
.../zig/simdjzon $ ../zig/stage4/bin/zig version
0.13.0-dev.9+21a3a5710
.../zig/simdjzon $ poop 'zig build-exe -fno-emit-bin -ODebug --dep simdjzon -Mroot=src/main.zig --dep build_options -Msimdjzon=src/simdjzon.zig -Mbuild_options=zig-cache/c/8caf24ab22f7fa50f13c4c948a6b8cca/options.zig --cache-dir zig-cache --global-cache-dir /home/travis/.cache/zig --name simdjzon' '../zig/stage4/bin/zig build-exe -fno-emit-bin -ODebug --dep simdjzon -Mroot=src/main.zig --dep build_options -Msimdjzon=src/simdjzon.zig -Mbuild_options=zig-cache/c/8caf24ab22f7fa50f13c4c948a6b8cca/options.zig --cache-dir zig-cache --global-cache-dir /home/travis/.cache/zig --name simdjzon'
Benchmark 1 (36 runs): zig build-exe -fno-emit-bin -ODebug --dep simdjzon -Mroot=src/main.zig --dep build_options -Msimdjzon=src/simdjzon.zig -Mbuild_options=zig-cache/c/8caf24ab22f7fa50f13c4c948a6b8cca/options.zig --cache-dir zig-cache --global-cache-dir /home/travis/.cache/zig --name simdjzon
  measurement          mean ± σ            min … max           outliers         delta
  wall_time           139ms ± 1.58ms     136ms …  144ms          1 ( 3%)        0%
  peak_rss            111MB ±  285KB     111MB …  112MB          1 ( 3%)        0%
  cpu_cycles          619M  ± 3.32M      612M  …  626M           4 (11%)        0%
  instructions        986M  ± 4.88K      986M  …  986M           2 ( 6%)        0%
  cache_references   58.8M  ±  356K     58.1M  … 59.5M           0 ( 0%)        0%
  cache_misses       8.02M  ±  106K     7.85M  … 8.24M           0 ( 0%)        0%
  branch_misses      4.60M  ± 28.0K     4.54M  … 4.68M           1 ( 3%)        0%
Benchmark 2 (41 runs): ../zig/stage4/bin/zig build-exe -fno-emit-bin -ODebug --dep simdjzon -Mroot=src/main.zig --dep build_options -Msimdjzon=src/simdjzon.zig -Mbuild_options=zig-cache/c/8caf24ab22f7fa50f13c4c948a6b8cca/options.zig --cache-dir zig-cache --global-cache-dir /home/travis/.cache/zig --name simdjzon
  measurement          mean ± σ            min … max           outliers         delta
  wall_time           123ms ± 1.24ms     121ms …  128ms          1 ( 2%)        ⚡- 11.5% ±  0.5%
  peak_rss            106MB ±  178KB     106MB …  106MB          2 ( 5%)        ⚡-  4.5% ±  0.1%
  cpu_cycles          465M  ± 3.15M      461M  …  479M           2 ( 5%)        ⚡- 24.8% ±  0.2%
  instructions        718M  ± 4.03K      718M  …  718M           0 ( 0%)        ⚡- 27.1% ±  0.0%
  cache_references   52.7M  ±  342K     52.1M  … 53.5M           0 ( 0%)        ⚡- 10.4% ±  0.3%
  cache_misses       7.56M  ±  133K     7.30M  … 7.96M           2 ( 5%)        ⚡-  5.8% ±  0.7%
  branch_misses      3.13M  ± 19.2K     3.08M  … 3.18M           3 ( 7%)        ⚡- 31.9% ±  0.2%

This seems too good to be true. Can anyone spot any mistakes or maybe try reproducing with this branch and building a different project?

travisstaloch · 2024-04-22T00:04:51Z

Perf point: building my simdjzon project with around 7000 loc zig files in src folder.

I got some feedback on discord from @jacobly0 @nektro and @Sinon (sorry forget your github handle).

if you want to benchmark code speed it's better to do it at runtime in a loop in an executable and benchmark the executable

The consensus I think is that this benchmark isn't reliable since there are too many moving parts such as caching. So I believe my previous benchmarks are a better measurement.

lib/std/static_string_map.zig

this patch renames ComptimeStringMap to StaticStringMap, makes it accept only a single type parameter, and return a known struct type instead of an anonymous struct. initial motivation for these changes was to reduce the 'very long type names' issue described here ziglang#19682. this breaks the previous API. users will now need to write: `const map = std.StaticStringMap(T).initComptime(kvs_list);` * move `kvs_list` param from type param to an `initComptime()` param * new public methods * `keys()`, `values()` helpers * `init(allocator)`, `deinit(allocator)` for runtime data * `getLongestPrefix(str)`, `getLongestPrefixIndex(str)` - i'm not sure these belong but have left in for now incase they are deemed useful * performance notes: * i posted some benchmarking results here: https://github.com/travisstaloch/comptime-string-map-revised/issues/1 * i noticed a speedup reducing the size of the struct from 48 to 32 bytes and thus use u32s instead of usize for all length fields * i noticed speedup storing KVs as a struct of arrays * latest benchmark shows these wall_time improvements for debug/safe/small/fast builds: -6.6% / -10.2% / -19.1% / -8.9%. full output in link above.

travisstaloch · 2024-04-22T08:33:29Z

lib/std/static_string_map.zig

+        pub inline fn initComptime(comptime kvs_list: anytype) Self {
+            comptime {
+                @setEvalBranchQuota(30 * kvs_list.len);


@Vexu done. This seems to be just big enough. I've added another test at bottom with a kvs_list of length 1000.

erikarvstedt · 2024-04-22T12:40:03Z

lib/std/static_string_map.zig

+        /// Returns the longest key, value pair where key is a prefix of `str`
+        /// else null.


Nit: This makes it sound as if the value contributes to the length calculation.

Suggested change

/// Returns the longest key, value pair where key is a prefix of `str`

/// else null.

/// Returns the key-value pair where key is the longest prefix of `str`

/// else null.

Thanks. 👍 . I'll make sure to change this next review if getLongestPrefix() is approved.

Ah merged already. Will have to be a follow up.

I've added these changes in #19823 along with a note about time complexity

andrewrk · 2024-04-22T22:27:40Z

EDIT: note to readers - this commit message is old Please read the current, updated one.

Can you edit your PR description to be correct so that I can use it for release notes in the future?

nice work, btw.

travisstaloch · 2024-04-22T22:33:19Z

Can you edit your PR description to be correct so that I can use it for release notes in the future?

Thanks! Done. And will do.

Not only did the name change, but the initialization procedure and types also needed to be tweaked. This made me realize that with my other memory management changes, the attempts to lookup shorthands for chromatic and whole-tone patterns was failing. That was due to the Pitch.asText() return value going out of scope, so it was always blank. Instead of trying to use a static array and be tricky, we'll pass in an allocator like I probably should have to begin with. See: - ziglang/zig#19719

ziglang/zig#19597 ziglang/zig#19460 ziglang/zig#19719

travisstaloch force-pushed the comptime-str-map-revised branch from 865a9a6 to 1953227 Compare April 21, 2024 08:07

travisstaloch requested a review from squeek502 as a code owner April 21, 2024 08:07

travisstaloch force-pushed the comptime-str-map-revised branch 2 times, most recently from 3237536 to 7ae12a5 Compare April 21, 2024 08:11

travisstaloch force-pushed the comptime-str-map-revised branch from 7ae12a5 to 7ad58ca Compare April 21, 2024 08:24

travisstaloch force-pushed the comptime-str-map-revised branch from 7ad58ca to 1d4d4dc Compare April 21, 2024 08:49

travisstaloch force-pushed the comptime-str-map-revised branch 2 times, most recently from cebaa3f to b7f3025 Compare April 21, 2024 10:51

travisstaloch force-pushed the comptime-str-map-revised branch from b7f3025 to 1e7fad0 Compare April 21, 2024 21:42

travisstaloch force-pushed the comptime-str-map-revised branch from 1e7fad0 to 21a3a57 Compare April 21, 2024 22:07

Vexu approved these changes Apr 22, 2024

View reviewed changes

lib/std/static_string_map.zig Outdated Show resolved Hide resolved

travisstaloch force-pushed the comptime-str-map-revised branch from 21a3a57 to ea50d26 Compare April 22, 2024 08:28

travisstaloch commented Apr 22, 2024

View reviewed changes

erikarvstedt reviewed Apr 22, 2024

View reviewed changes

andrewrk added standard library This issue involves writing Zig code for the standard library. release notes This PR should be mentioned in the release notes. labels Apr 22, 2024

andrewrk merged commit 8af59d1 into ziglang:master Apr 22, 2024
10 checks passed

travisstaloch deleted the comptime-str-map-revised branch April 23, 2024 03:03

Froxcey mentioned this pull request Apr 25, 2024

Replace ComptimeStringMap with StaticStringMap jetzig-framework/zmd#1

Closed

This was referenced May 2, 2024

ComptimeStringMap replaced with StaticStringMap ZigEmbeddedGroup/microzig#185

Closed

ComptimeStringMap replaced with StaticStringMap ZigEmbeddedGroup/microzig#186

Closed

MFAshby added a commit to MFAshby/zig-lsp-kit that referenced this pull request May 23, 2024

Fixups for the following changes in zig:

1c07e3e

ziglang/zig#19597 ziglang/zig#19460 ziglang/zig#19719

Froxcey mentioned this pull request May 30, 2024

New projects made with jetzig init on Windows will not run jetzig-framework/jetzig#81

Closed

travisstaloch mentioned this pull request Jun 20, 2024

std.static_string_map: Cast length of input for runtime initialization #20350

Merged

travisstaloch mentioned this pull request Sep 23, 2024

Replace StaticStringMap with far more optimized version #21498

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ComptimeStringMap: return a regular struct and optimize #19719

ComptimeStringMap: return a regular struct and optimize #19719

travisstaloch commented Apr 21, 2024 •

edited

Loading

Vexu commented Apr 21, 2024

travisstaloch commented Apr 21, 2024 •

edited

Loading

travisstaloch commented Apr 21, 2024

travisstaloch commented Apr 21, 2024

travisstaloch commented Apr 21, 2024 •

edited

Loading

travisstaloch commented Apr 21, 2024

BratishkaErik commented Apr 21, 2024

travisstaloch commented Apr 21, 2024

Vexu commented Apr 21, 2024

travisstaloch commented Apr 21, 2024

erikarvstedt commented Apr 21, 2024 •

edited

Loading

travisstaloch commented Apr 21, 2024

travisstaloch commented Apr 21, 2024

travisstaloch commented Apr 21, 2024

travisstaloch commented Apr 22, 2024 •

edited

Loading

travisstaloch Apr 22, 2024

erikarvstedt Apr 22, 2024

travisstaloch Apr 22, 2024

travisstaloch Apr 22, 2024

travisstaloch May 1, 2024

andrewrk commented Apr 22, 2024 •

edited

Loading

travisstaloch commented Apr 22, 2024

		/// Returns the longest key, value pair where key is a prefix of `str`
		/// else null.

ComptimeStringMap: return a regular struct and optimize #19719

ComptimeStringMap: return a regular struct and optimize #19719

Conversation

travisstaloch commented Apr 21, 2024 • edited Loading

Vexu commented Apr 21, 2024

travisstaloch commented Apr 21, 2024 • edited Loading

travisstaloch commented Apr 21, 2024

travisstaloch commented Apr 21, 2024

travisstaloch commented Apr 21, 2024 • edited Loading

travisstaloch commented Apr 21, 2024

BratishkaErik commented Apr 21, 2024

travisstaloch commented Apr 21, 2024

Vexu commented Apr 21, 2024

travisstaloch commented Apr 21, 2024

erikarvstedt commented Apr 21, 2024 • edited Loading

travisstaloch commented Apr 21, 2024

travisstaloch commented Apr 21, 2024

travisstaloch commented Apr 21, 2024

travisstaloch commented Apr 22, 2024 • edited Loading

travisstaloch Apr 22, 2024

Choose a reason for hiding this comment

erikarvstedt Apr 22, 2024

Choose a reason for hiding this comment

travisstaloch Apr 22, 2024

Choose a reason for hiding this comment

travisstaloch Apr 22, 2024

Choose a reason for hiding this comment

travisstaloch May 1, 2024

Choose a reason for hiding this comment

andrewrk commented Apr 22, 2024 • edited Loading

travisstaloch commented Apr 22, 2024

travisstaloch commented Apr 21, 2024 •

edited

Loading

travisstaloch commented Apr 21, 2024 •

edited

Loading

travisstaloch commented Apr 21, 2024 •

edited

Loading

erikarvstedt commented Apr 21, 2024 •

edited

Loading

travisstaloch commented Apr 22, 2024 •

edited

Loading

andrewrk commented Apr 22, 2024 •

edited

Loading