Remove internal uses of `withForeignPtr` #536

clyring · 2022-07-29T23:25:36Z

As discussed at #535, withForeignPtr will be much slower in at least GHC-9.2.4 and GHC-9.4.1. So I made a quick pass at removing all internal uses of this function. To do this, I added variants of the create family of functions whose "action" arguments take ForeignPtr Word8 instead of Ptr Word8, allowing the caller to decide whether to use withForeignPtr or unsafeWithForeignPtr. Then I used those ForeignPtr variants everywhere.

I haven't yet had time to investigate whether this actually helps performance with 9.2.4 and obviously for a release I should need to do this against the 0.11 branch. But I thought I should push this for review now anyway.

Fixes #461.

clyring · 2022-07-30T10:37:55Z

My machine is giving me a very hard time, so I might not be able to continue work on this for a while. The timing is unfortunate. Maintainers, please feel free to push review suggestions to my branch as time is scarce.

A few other thoughts:

I'm not attached to the names of the helper functions I introduced.
Do we want to export these helper functions from D.B.Internal? This is always awkward. Anything we don't want to export from there can be moved to a new module like D.B.Internal.Utils which {-# SOURCE #-} imports D.B.Internal.
There are a few bytestring-internal uses of create that can't be safely replaced by create_but_with_unsafeWithForeignPtr. One of these is scanl, already handled correctly in this patch. It looks like I missed filter in my first pass. Are there others?

Bodigrim · 2022-07-30T22:01:05Z

Sorry, I’m AFK atm, will take a look tomorrow night.

Bodigrim · 2022-07-31T21:52:53Z

I'm not attached to the names of the helper functions I introduced.

Do we want to export these helper functions from D.B.Internal? This is always awkward. Anything we don't want to export from there can be moved to a new module like D.B.Internal.Utils which {-# SOURCE #-} imports D.B.Internal.

Yeah, I'd prefer to hide these helpers from users, D.B.Internal.Utils is a good location. And hiding them from users would save us bikeshedding of names.

The error messages from ghc-8.0 weren't red, so I didn't notice them. Silly stuff.

clyring · 2022-08-07T02:15:10Z

I was finally able to work on this again tonight. The diff is rather large, but the majority of it is a very mechanical translation from code that uses Ptr to code that uses ForeignPtr via the new helper functions. So I hope it is easy to review despite its size.

I have moved the new helper functions into Data.ByteString.Internal.Utils, so there is no change to the exposed API.
I audited every use of unsafeWithForeignPtr in the library, and adjusted every use site that didn't look "obviously always terminating."
I briefly looked into backporting this to the 0.11 branch; there is a conflict since Improve memory-safety of Semigroup instances #443 has not been backported. But it's probably easy to fix, and we could also just backport Improve memory-safety of Semigroup instances #443 first.

Data/ByteString/Internal.hs-boot

Data/ByteString/Internal/Utils.hs

bgamari · 2022-08-07T18:59:15Z

Thank you for doing this, @clyring. For what it's worth, I agree with @Bodigrim regarding naming. The lower-case fp and f suffixes make for names that are very hard to visually parse.

This reverts commit b6b4de6. Revert "Move unsafeWithForeignPtr shim into Utils" This reverts commit 6df394e. Revert "Move new utilities into D.B.Internal.Utils" This reverts commit 6462e4f.

to Data.ByteString.Internal.Type. The re-export-only compatibility module to replace it at Data.ByteString.Internal is intentionally deleted from this commit, to allow for a more reviewable diff.

Bodigrim

Thanks @clyring, insane amount of work!

sjakobi

Thanks a lot, @clyring!

One minor wibble.

Data/ByteString/Internal/Type.hs

clyring · 2022-08-22T01:29:55Z

ping @sjakobi

sjakobi · 2022-08-22T19:24:23Z

Apologies for the delay, @clyring! I intend to get back to this at the end of this week.

BTW, has anyone investigated the perf impact already?

I haven't yet had time to investigate whether this actually helps performance with 9.2.4 and obviously for a release I should need to do this against the 0.11 branch.

Otherwise I'll do that when I'm back.

clyring · 2022-08-22T22:57:04Z

BTW, has anyone investigated the perf impact already?

I haven't yet had time to investigate whether this actually helps performance with 9.2.4 and obviously for a release I should need to do this against the 0.11 branch.

Otherwise I'll do that when I'm back.

I tried measuring the effect on runtime via our benchmarks but the noise in my attempts was greater than the effect size.

I also just tried running our entire testsuite with --quickcheck-replay=953718 +RTS -s and found this branch consistently gets 0.6% fewer "bytes allocated in the heap" than does master. That seems promising, but I'm sure it's not a complete picture.

It has the same performance issues as does withForeignPtr.

clyring · 2022-08-26T02:29:02Z

I've verified that the improvement in runtime allocations is about 10% for the relevant GHC perf test. That test also allocates a bunch of list objects, so the improvement when looking at "only" bytestring stuff is probably a fair bit larger. (But I don't plan to investigate this any further.)

PS: Thanks sjakobi for bringing the performance question back up.

sjakobi · 2022-08-30T11:40:22Z

I've started comparing the benchmark results with this patch and GHC 9.2.4 vs. master with GHC 9.2.3. On first sight, this patch did not seem to recover a 2x slowdown in some of the Builder benchmarks. I'll take a closer look tomorrow or on Thursday.

sjakobi · 2022-08-30T11:55:26Z

Here are some examples of persistent slowdowns (comparing master with 9.2.3 (OLD) vs. this patch with 9.2.4 (NEW)):

Name,Old,New,Ratio
All.Data.ByteString.Builder.Encoding wrappers.foldMap word8 (10000),61623738,113407945,1.84033
All.Data.ByteString.Builder.ByteString insertion.foldMap byteStringInsert byteStringChunks4Data (10000),266916778,323701687,1.21274
All.Data.ByteString.Builder.ByteString insertion.foldMap byteString byteStringChunks4Data (10000),50016262,99267230,1.9847
All.Data.ByteString.Builder.ByteString insertion.foldMap byteStringCopy byteStringChunks4Data (10000),50212852,97953266,1.95076

Bodigrim · 2022-09-03T23:31:08Z

(Off topic: @clyring would you mind to drop me an email? I have a question to discuss, but cannot find your contacts)

Bodigrim · 2022-09-17T16:49:41Z

Not sure what else we can do here. If this branch is better than status quo, let’s merge as is? I’d like to push out 0.11.4.0 soon.

clyring · 2022-09-18T12:49:58Z

I'd like to understand the Builder regressions a bit better. But they seem likely unrelated to the expected keepAlive# regressions this patch aims to fix, and shouldn't block it.

Later today I will try to reproduce the 9.2.4 Builder regressions and inquire about them at GHC's issue tracker.

clyring · 2022-09-18T22:22:11Z

I was able to reproduce; it's rather terrible. byteStringHex gets 10x slower for me. That particular regression seems to happen because GHC is no longer eta-expanding something it probably should. I created this GHC issue on the matter.

sjakobi

Sorry for the late response, guys! Indeed the regressions seem orthogonal to this patch.

Also, thanks a lot for all your work on bytestring and taking on maintainer duties, @clyring! :)

* Avoid 'withForeignPtr' via 'createf' and variants * remove stupid/wrong temporary cpp * Move new utilities into D.B.Internal.Utils * Move unsafeWithForeignPtr shim into Utils * Fix imports for unsafeWithForeignPtr shim The error messages from ghc-8.0 weren't red, so I didn't notice them. Silly stuff. * Clean up potentially-sketchy unsafeWithForeignPtr uses * Revert documentation change to createAndTrim * Revert "Fix imports for unsafeWithForeignPtr shim" This reverts commit b6b4de6. Revert "Move unsafeWithForeignPtr shim into Utils" This reverts commit 6df394e. Revert "Move new utilities into D.B.Internal.Utils" This reverts commit 6462e4f. * Rename ForeignPtr helpers to use "Fp" suffix * [NONBUILDABLE] rename Data.ByteString.Internal to Data.ByteString.Internal.Type. The re-export-only compatibility module to replace it at Data.ByteString.Internal is intentionally deleted from this commit, to allow for a more reviewable diff. * Add compatibility stub "Data.ByteString.Internal" * Replace Data.ByteString.Internal.Type's description * Remove the use of withRawBuffer, too. It has the same performance issues as does withForeignPtr. Co-authored-by: Matthew Craven <[email protected]> # Conflicts: # Data/ByteString/Char8.hs # Data/ByteString/Internal.hs

clyring added 2 commits July 29, 2022 15:27

Avoid 'withForeignPtr' via 'createf' and variants

57173e0

remove stupid/wrong temporary cpp

5c2e72d

clyring added 5 commits August 6, 2022 20:06

Move new utilities into D.B.Internal.Utils

6462e4f

Move unsafeWithForeignPtr shim into Utils

6df394e

Fix imports for unsafeWithForeignPtr shim

b6b4de6

The error messages from ghc-8.0 weren't red, so I didn't notice them. Silly stuff.

Clean up potentially-sketchy unsafeWithForeignPtr uses

4d59766

Revert documentation change to createAndTrim

f933ccf

Bodigrim reviewed Aug 7, 2022

View reviewed changes

Data/ByteString/Internal.hs-boot Outdated Show resolved Hide resolved

Bodigrim reviewed Aug 7, 2022

View reviewed changes

Data/ByteString/Internal/Utils.hs Outdated Show resolved Hide resolved

clyring added 4 commits August 7, 2022 16:36

Revert "Fix imports for unsafeWithForeignPtr shim"

0d4f106

This reverts commit b6b4de6. Revert "Move unsafeWithForeignPtr shim into Utils" This reverts commit 6df394e. Revert "Move new utilities into D.B.Internal.Utils" This reverts commit 6462e4f.

Rename ForeignPtr helpers to use "Fp" suffix

698faec

[NONBUILDABLE] rename Data.ByteString.Internal

299e7ac

to Data.ByteString.Internal.Type. The re-export-only compatibility module to replace it at Data.ByteString.Internal is intentionally deleted from this commit, to allow for a more reviewable diff.

Add compatibility stub "Data.ByteString.Internal"

15cbde9

Bodigrim approved these changes Aug 12, 2022

View reviewed changes

Bodigrim requested a review from sjakobi August 12, 2022 20:26

sjakobi reviewed Aug 14, 2022

View reviewed changes

Data/ByteString/Internal/Type.hs Outdated Show resolved Hide resolved

sjakobi mentioned this pull request Aug 14, 2022

Use unsafeWithForeignPtr in create* functions. #535

Closed

Replace Data.ByteString.Internal.Type's description

eee138c

Remove the use of withRawBuffer, too.

b498c4d

It has the same performance issues as does withForeignPtr.

Merge branch 'master' into no-withForeignPtr

040df6f

sjakobi approved these changes Sep 26, 2022

View reviewed changes

Bodigrim merged commit 3b0f5d3 into haskell:master Sep 26, 2022

Bodigrim added this to the 0.11.4.0 milestone Sep 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove internal uses of `withForeignPtr` #536

Remove internal uses of `withForeignPtr` #536

clyring commented Jul 29, 2022 •

edited

Loading

clyring commented Jul 30, 2022

Bodigrim commented Jul 30, 2022

Bodigrim commented Jul 31, 2022

clyring commented Aug 7, 2022

bgamari commented Aug 7, 2022

Bodigrim left a comment

sjakobi left a comment

clyring commented Aug 22, 2022

sjakobi commented Aug 22, 2022

clyring commented Aug 22, 2022

clyring commented Aug 26, 2022 •

edited

Loading

sjakobi commented Aug 30, 2022

sjakobi commented Aug 30, 2022

Bodigrim commented Sep 3, 2022

Bodigrim commented Sep 17, 2022

clyring commented Sep 18, 2022

clyring commented Sep 18, 2022

sjakobi left a comment

Remove internal uses of withForeignPtr #536

Remove internal uses of withForeignPtr #536

Conversation

clyring commented Jul 29, 2022 • edited Loading

clyring commented Jul 30, 2022

Bodigrim commented Jul 30, 2022

Bodigrim commented Jul 31, 2022

clyring commented Aug 7, 2022

bgamari commented Aug 7, 2022

Bodigrim left a comment

Choose a reason for hiding this comment

sjakobi left a comment

Choose a reason for hiding this comment

clyring commented Aug 22, 2022

sjakobi commented Aug 22, 2022

clyring commented Aug 22, 2022

clyring commented Aug 26, 2022 • edited Loading

sjakobi commented Aug 30, 2022

sjakobi commented Aug 30, 2022

Bodigrim commented Sep 3, 2022

Bodigrim commented Sep 17, 2022

clyring commented Sep 18, 2022

clyring commented Sep 18, 2022

sjakobi left a comment

Choose a reason for hiding this comment

Remove internal uses of `withForeignPtr` #536

Remove internal uses of `withForeignPtr` #536

clyring commented Jul 29, 2022 •

edited

Loading

clyring commented Aug 26, 2022 •

edited

Loading