Skip to content

Make Size fields strict #456

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 10, 2023
Merged

Make Size fields strict #456

merged 2 commits into from
Mar 10, 2023

Conversation

konsumlamm
Copy link
Contributor

Closes #452.

See #452 (comment) for a benchmark comparison (although note that the benchmark results vary quite a bit). Maybe I'm missing something and there's a reason the fields aren't strict, but it doesn't look that way to me.

@chessai
Copy link
Member

chessai commented Mar 9, 2023

This looks right to me. And we could add {-# UNPACK #-} as well. IIRC strict fields should be unpacked when compiling with -O2 automatically, but IMO it doesn't hurt to add.

@konsumlamm
Copy link
Contributor Author

IIRC strict fields should be unpacked when compiling with -O2 automatically, but IMO it doesn't hurt to add.

Yes, they're even unpacked with -O1 (see -funbox-small-strict-fields). But all the other strict fields also have {-# UNPAPCK #-} pragmas, so ig it makes sense to add it here too.

@lehins lehins merged commit 85e14d7 into haskell:master Mar 10, 2023
@lehins
Copy link
Contributor

lehins commented Mar 10, 2023

Thank you for this change. I wanted to make Size strict for for a while, but I always thought it was maybe lazy on purpose, potentially due to occasional case where size calculation is discarded. After dedicating a bit of time investigating this theory due to this PR I couldn't really find any function that could benefit from such laziness. Of course, this investigation was by no means exhaustive. In any case, if there is such place where size calculation turns out to be redundant, majority of the time it is better to not pay the price for thunks.

@konsumlamm konsumlamm deleted the size branch March 10, 2023 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data.Vector.Fusion.Bundle.Size.Size fields are not strict
3 participants