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

Avoid boxing arrays (#230 rebased) #375

Closed
wants to merge 1 commit into from
Closed

Conversation

sjakobi
Copy link
Member

@sjakobi sjakobi commented Mar 12, 2022

Previously, we used a lot of things that looked like

runST (act >>= unsafeFreeze)

The trouble is that GHC can't unbox past the runRW# primitive
that runST is based on. So this actually allocates an Array
constructor which we will then throw away immediately.

The way to avoid this is to call runRW# manually to produce
a raw SmallArray#, then apply the Array constructor on the
outside.

@sjakobi sjakobi mentioned this pull request Mar 12, 2022
@treeowl
Copy link
Collaborator

treeowl commented Mar 12, 2022

Let's make sure this works decently with recent GHC. Some things have gotten harder. Sigh

Data/HashMap/Internal.hs Outdated Show resolved Hide resolved
Data/HashMap/Internal/Array.hs Outdated Show resolved Hide resolved
Comment on lines +270 to +280
#if MIN_VERSION_base(4,9,0)
-- GHC can't unbox across the runRW# boundary, so we apply the Array constructor
-- on the outside.
run (ST act) =
case runRW# $ \s ->
case act s of { (# s', MArray mary #) ->
unsafeFreezeSmallArray# mary s' } of
(# _, ary #) -> Array ary
#else
run act = runST (act >>= unsafeFreeze)
#endif
Copy link
Member Author

Choose a reason for hiding this comment

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

So far I haven't seen any benefit from this change so far with GHC 9.2.2, but I'll check again once #376 and #377 are merged.

@sjakobi
Copy link
Member Author

sjakobi commented Mar 12, 2022

Let's make sure this works decently with recent GHC. Some things have gotten harder. Sigh

I like what I've seen so far in the Core. The only inner Array boxing that's left is in the traverse[WithKey] code, and I'm not sure whether that's avoidable.

I probably won't bother to look at Core diffs across GHC versions, so if you're suspecting that some things that this patch used to do don't work anymore with recent GHCs, you'll have to figure this out yourself.

Previously, we used a lot of things that looked like

    runST (act >>= unsafeFreeze)

The trouble is that GHC can't unbox past the `runRW#` primitive
that `runST` is based on. So this actually allocates an `Array`
constructor which we will then throw away immediately.

The way to avoid this is to call `runRW#` manually to produce
a raw `SmallArray#`, then apply the `Array` constructor on the
outside.
@sjakobi
Copy link
Member Author

sjakobi commented Mar 15, 2022

After merging #376 and #377 and rebasing, I can't find any significant changes to Core and STG as a result of these remaining changes.

@treeowl Is there any reason to merge (parts of) this?

@sjakobi sjakobi closed this Mar 22, 2022
@sjakobi sjakobi deleted the sjakobi/unbox-arrays branch March 22, 2022 04:08
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.

2 participants