refactor(stdune): improve path tables

[rgrinberg]

Use a more memory efficient path table. Instead of using the variant for
the key, combine 3 tables all for the individual paths.

This makes the empty table a little more bloated (3x bigger), but gives
us a saving of 2 words for every single key we store.

Signed-off-by: Rudi Grinberg [email protected]

[snowleopard]

Looks good! I am going to benchmark this change internally too.

[snowleopard]

What about Path.Set and Path.Map? They are probably even bigger offenders (because we have more of them).

It's fine to leave them for another PR, just wondering if you've got plans.

[rgrinberg]

What about Path.Set and Path.Map? They are probably even bigger offenders (because we have more of them).

I have an upcoming PR indeed.

[snowleopard]

The current version of this PR still shows as a slight regression in my benchmarks.

@rgrinberg What are the results on your end?

[rgrinberg]

Runtime performance shows a slight improvement but it's well within the noise range.

The GC stats are quite accurate and they do show a small improvement across the board, but that isn't necessarily better for performance.

[snowleopard]

Runtime performance shows a slight improvement but it's well within the noise range.

The GC stats are quite accurate and they do show a small improvement across the board, but that isn't necessarily better for performance.

Hmm, I wonder if we're seeing different results because we're using different sets of rules.

I'm also somewhat surprised to see a small regression in my benchmarks. It seems like this PR is mostly changing things for the better, so I'm kind of puzzled.

[rgrinberg]

I'm not surprised that this change isn't a performance improvement, but I'm definitely surprised there's any regression. I imagined there would be no difference at all either way. I guess we should remember that OCaml hash tables aren't particularly space efficient anyway and removing a single word isn't going to change all that much.

We should note that our benchmarks are a bit biased against watch mode though. In watch mode, consuming less memory is more important because it makes subsequent operations faster. While in our measurements, we only care about the first completion build and do not care about any subsequent memory usage.

[snowleopard]

Just a quick update: I've started some more benchmarks.

[snowleopard]

Hah, now I'm seeing pretty consistent small improvements across most benchmarks! Maybe I botched it the first time.

Roughly: -0.1-0.2% in terms of allocations and -0.1-0.5% in terms of time.

[snowleopard]

Thanks!

[snowleopard]

@rgrinberg Btw, don't forget to switch to ~store:(module Path.Table) in Action_builder.contents. I'm making this change internally.

[snowleopard]

@rgrinberg Btw, don't forget to switch to ~store:(module Path.Table) in Action_builder.contents. I'm making this change internally.

Just to add: with this change included, benchmarks now 2% speed-up for from-cache and null builds!