Skip to content

Use unsafeWithForeignPtr where possible #372

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 1 commit into from
Mar 17, 2021

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented Mar 15, 2021

In GHC 9.0.1 withForeignPtr became (necessarily) considerably more
expensive. In GHC ticket #19474, it was noted that Storable Vectors
are affected by this regression. Fix this by using
unsafeWithForeignPtr when it is known that the continuation is
not divergent.

See also GHC #17760.

In GHC 9.0.1 `withForeignPtr` became (necessarily) considerably more
expensive. In GHC ticket #19474, it was noted that `Storable` `Vector`s
are affected by this regression. Fix this by using
`unsafeWithForeignPtr` when it is known that the continuation is
not divergent.

See also GHC #17760.
@lehins lehins self-requested a review March 16, 2021 00:25
Copy link
Contributor

@lehins lehins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bgamari I didn't realize you finished keepAlive# for 9.0.1 Thank you for doing the work and for submitting this PR.

I think we also need to use unsafeWithForeignPtr in:

  • Data.Vector.Storable.unsafeWith
  • Data.Vector.Storable.Mutable.unsafeWith

They both are already unsafe, so we just need to add a note to documentation that the supplied action must terminate.

@bgamari Do you think you can add it to this PR? I can do it later myself of course, unless you have an argument against using unsafeWithForeignPtr in those functions.

@bgamari
Copy link
Contributor Author

bgamari commented Mar 16, 2021

They both are already unsafe, so we just need to add a note to documentation that the supplied action must terminate.

Yes, I saw these but I'll admit I was a bit hesitant to extend the new behavior to unsafeWith given its somewhat hard-to-predict effects. That being said, I'm not opposed to doing so assuming we adequately document the change.

@Shimuuar
Copy link
Contributor

LGTM. This looks quite bad so I think this should backported to 0.12 branch after PR is merged.

@lehins
Copy link
Contributor

lehins commented Mar 16, 2021

I think this should backported to 0.12 branch after PR is merged.

100% agree.

@sjakobi
Copy link
Member

sjakobi commented Mar 17, 2021

I'm curious: Could this perf bug have been caught be comparing vector's benchmark results between GHC 8.10 and GHC 9.0.1?

@Bodigrim Bodigrim merged commit 9a13bcb into haskell:master Mar 17, 2021
@Bodigrim
Copy link
Contributor

@sjakobi several benchmarks got 5-6% slower, but this is not that much alarming on its own.

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.

5 participants