Run alter in one pass

[oberblastmeister]

Resolves #404

[sjakobi]

Thanks! I have only minor comments! :)

Please update Strict.alter and Strict.update similarly. I'm curious what impact this will have on the benchmarks.

[sjakobi]

If we use two for Collision nodes, we'll need to update its documentation. Could you do that?

#447 is related.

[oberblastmeister]

What should I put in the documentation? I think the function may also need a more descriptive name, like bitmapIndexedFromTwo or something.

[oberblastmeister]

I bet we could implement delete in terms of alter with some inlining.

[sjakobi]

I bet we could implement delete in terms of alter with some inlining.

Probably. My inclination is not to rely on GHC optimizations more than necessary though.

Also, wouldn't this increase the size of the unfolding and thereby make it less likely that delete is inlined in user code?!

[oberblastmeister]

My inclination is not to rely on GHC optimizations more than necessary though.

I think these are very simple optimizations, beta reduction and case simplification, which are performed by the ghc simplifier, and I am pretty sure these are very likely to fire. We also rely on these sorts of optimizations throughout the library anyways. It doesn't matter that much though, but would reduce a lot of code duplication, as alter is pretty much a direct copy of delete.

Why would this increase the size of the unfolding?

[sjakobi]

Sorry for the late response!

My inclination is not to rely on GHC optimizations more than necessary though.

I think these are very simple optimizations, beta reduction and case simplification, which are performed by the ghc simplifier, and I am pretty sure these are very likely to fire. We also rely on these sorts of optimizations throughout the library anyways. It doesn't matter that much though, but would reduce a lot of code duplication, as alter is pretty much a direct copy of delete.

I think this means that GHC will need to spend more cycles on optimizing usage of delete in user code. So the effect will be an increase in compile time and possibly a failure to perform other optimization once GHC runs out of simplifier iterations.

Feel free to give it a try in a follow-up PR though. I'm curious what effects this will have on the Core for delete.

Why would this increase the size of the unfolding?

My bad. Briefly, I had thought that the unfolding for delete would contain the unoptimized unfolding of alter.

[oberblastmeister]

I can't really tell if the benchmarks get faster or not, which is a bit weird.

[oberblastmeister]

I wonder if the old alter inlines, thus removing the closure and simplifying the cases. I am pretty sure the new alter will never inline.