Skip to content
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

Uncons should not produce thunks #558

Open
clyring opened this issue Nov 23, 2022 · 1 comment · Fixed by #559
Open

Uncons should not produce thunks #558

clyring opened this issue Nov 23, 2022 · 1 comment · Fixed by #559
Labels
blocked: ghc This is blocked on a feature or primitive not yet available in a released GHC version performance

Comments

@clyring
Copy link
Member

clyring commented Nov 23, 2022

Consider Lazy.uncons. Today, we have in Data.ByteString.Lazy:

uncons :: ByteString -> Maybe (Word8, ByteString)
uncons Empty = Nothing
uncons (Chunk c cs)
    = Just (S.unsafeHead c,
            if S.length c == 1 then cs else Chunk (S.unsafeTail c) cs)

Both components of the returned tuple internally perform a small amount of work before constructing a result:

  • The first component has to read from the underlying buffer before it can construct its I# result. Since the compiler doesn't know that this read should not fail, it cannot perform the read eagerly; it must create a thunk. (The StrictByteString version of uncons has the same issue here.)
  • The second component has to actually perform the if-then-else before either evaluating cs or allocating a new Chunk object. This is just an comparison between evaluated Ints, and certainly succeeds in little time, so a compiler would be well within its rights to eagerly compare S.length c with 1, but GHC today does not do so for what-ever reason.

What should we do about this?

  • We could make uncons strict in S.unsafeHead c, but then the resulting readWord8OffAddr# read-effect operations will stick around even if the result is un-used, which is a bit unfortunate. (Maybe Cmm assignment sinking or the register allocator can clean it up very late in the pipeline. I haven't checked.) And users pattern-matching on the result of uncons have an easy work-around: Use a bang-pattern themselves. So I'm not convinced this behavior should be changed before GHC starts to discard unused read-effects in its simplifier.
  • I think we definitely should perform the S.length check eagerly: Since the thunk that would be allocated is one word larger than the Chunk cell allocated in the else branch, I expect eagerly performing this comparison to be cheaper on average even if the thunk would never be evaluated. And users pattern-matching on the result of uncons have no comfortable workaround, since a bang-pattern may force the lazy tail prematurely. I will put up a merge request implementing this momentarily.
@clyring clyring added blocked: ghc This is blocked on a feature or primitive not yet available in a released GHC version performance labels Nov 23, 2022
clyring added a commit to clyring/bytestring that referenced this issue Nov 23, 2022
...at least for the second component of its result.
The thunk for the first component is a bit hairier because
the readWord8OffAddr# is considered a side effect by GHC
and hence can currently never go away unless it is used lazily.

See also haskell#558.
@Bodigrim Bodigrim linked a pull request Nov 24, 2022 that will close this issue
clyring added a commit that referenced this issue Nov 25, 2022
...at least for the second component of its result.
The thunk for the first component is a bit hairier because
the readWord8OffAddr# is considered a side effect by GHC
and hence can currently never go away unless it is used lazily.

See also #558.
@clyring
Copy link
Member Author

clyring commented Nov 25, 2022

The problem with the second component of Lazy.uncons was fixed in #559. But the first component of the result of uncons is still a thunk, so I will re-open.

@clyring clyring reopened this Nov 25, 2022
Bodigrim pushed a commit to Bodigrim/bytestring that referenced this issue Nov 26, 2022
...at least for the second component of its result.
The thunk for the first component is a bit hairier because
the readWord8OffAddr# is considered a side effect by GHC
and hence can currently never go away unless it is used lazily.

See also haskell#558.
vdukhovni pushed a commit to vdukhovni/bytestring that referenced this issue Dec 7, 2022
...at least for the second component of its result.
The thunk for the first component is a bit hairier because
the readWord8OffAddr# is considered a side effect by GHC
and hence can currently never go away unless it is used lazily.

See also haskell#558.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked: ghc This is blocked on a feature or primitive not yet available in a released GHC version performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant