optimization: avoid NotGiven allocations

[fwbrasil]

Problem

I've been working on a library that uses NotGiven in the hot path, which ends up producing an extra allocation on every use. I thought JIT compilation would be able to optimize it away but that isn't the case in some benchmarks. For example:

Benchmark: https://github.com/fwbrasil/kyo/blob/main/kyo-bench/src/main/scala/kyo/bench/NarrowBindMapBench.scala
Flamegraph: https://getkyo.io/dev/profile/4990c20/kyo.bench.NarrowBindMapBench.forkKyo-Throughput/flame-cpu-forward.html

Solution

Make NotGiven.value a val instead of def.

Notes

• I'm assuming the identity of a NotGiven object isn't relevant.
• Should I use an internal field and keep NotGiven.value as def to avoid binary compatibility issues?

[fwbrasil]

I've just signed the CLA

[smarter]

Does it actually need a minor release? This seems to be source/tasty/binary-compatible /cc @sjrd.

[dwijnand]

We discussed this yesterday. It's not forwards tasty compatible, is the concern.

[smarter]

Ah, maybe tasty deserialization could be enhanced to make that kind of change actually compatible?

[sjrd]

It's fundamentally incompatible to go from a val to a def, because it makes it unstable. There's no "making it compatible". So going the other direction is not forwards compatible.

But you can make it compatible in all directions by storing the instance in a private val, and reading that val in the public def.

[smarter]

Ah yes, I didn't think of stability, I agree that a def pointing to a private val is the way to go.

[fwbrasil]

Thank you for the feedback! I've just updated the PR to use a private value

[smarter]

Thanks!

[dwijnand]

We also discussed going this way on Monday and had decided to just wait for the next minor and make it a val....

[dwijnand]

For these cases (in this instance it's not a big deal) but also personally, I'd love to see you both at the Monday team meetings.

[sjrd]

Yes, I should come more often.