Behavior differences between `--release` and dev profile

[Adrien-Bodineau]

A note for the community

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

Problem

here is an example from the disk buffer ledger https://github.com/vectordotdev/vector/blob/master/lib/vector-buffers/src/variants/disk_v2/ledger.rs#L127

        (self.get_current_writer_file_id() + 1) % MAX_FILE_ID

the + 1 can panic in dev mode but will silently wrap in release mode.

it would be best to unify behavior by using the specific addition function from the u16 type. This would add clarity on the behavior and how overflow is handled by specifying it explicitly.

Configuration

No response

Version

branch master

Debug Output

No response

Example Data

No response

Additional Context

No response

References

No response

[jszwedko]

Thanks for opening this @Adrien-Bodineau !

Are there any other differences between dev and release profiles you are interested in rectifying besides arithmetic overflow? I agree that it would be preferable to have Vector panic when there is arithmetic overflow. This would be achievable by setting overflow-checks to true in the release profile (it defaults to false).

[jszwedko]

Are there any other differences between dev and release profiles you are interested in rectifying besides arithmetic overflow? I agree that it would be preferable to have Vector panic when there is arithmetic overflow. This would be achievable by setting overflow-checks to true in the release profile (it defaults to false).

Actually, I forgot the trade-off is that it introduces overhead to every arithmetic operation. I'll open a PR setting it to see what the benchmarks show.

[jszwedko]

Opened PR to get an idea of performance impact from the CI benchmarks: https://github.com/vectordotdev/vector/pull/21164#pullrequestreview-2264090498

[Adrien-Bodineau]

I do not think it should always panic, depending on the case. in the ledger for example it seems that wrapping is the expected behavior and thus should be explicitly stated using wrapping_add() or the Wrapping type

[Adrien-Bodineau]

also worth to mention that in release the % MAX_FILE_ID does extra computation for nothing as it tries to compute the modulo u16::MAX. But because the value silently wrap in release the modulo operation is useless.

[Adrien-Bodineau]

for now the dev mode do not panic because MAX_FILE_ID is 6

[jszwedko]

I do not think it should always panic, depending on the case. in the ledger for example it seems that wrapping is the expected behavior and thus should be explicitly stated using wrapping_add() or the Wrapping type

Right, I should have been clearer that in cases where there might be wraparound it would be desirable that the code handles it explicitly. However, enabling overflow detection as exists in the dev profile would cause it to panic in cases where checked arithmetic wasn't used and overflow occurred. The question is if that is better than silent overflow. In general I think it is. The trade-off, however, is that I think performance will be negatively impacted. I'm still trying to run the benchmarks over on https://github.com/vectordotdev/vector/pull/21164 to get a concrete idea of the overhead.