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.

Sign up for GitHub

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

Jump to bottom

Tests and benchmarks for {Set,Map}.fromAscList and friends #962

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Tests and benchmarks for {Set,Map}.fromAscList and friends #962

wants to merge 1 commit into from

Conversation

meooow25
Copy link
Contributor

@meooow25 meooow25 commented Jul 14, 2023 •
edited
Loading

I wrote these up while working on making fromAscList fuse well. But I'm seeing some odd behavior there, so I thought I would split this straightforward PR out and work on the rest afterwards.

Current benchmarks on GHC 9.2.5
Set:

  fromAscList:  OK (0.21s)
    50.1 μs ± 2.7 μs, 240 KB allocated, 2.4 KB copied, 8.0 MB peak memory
  fromDescList: OK (0.21s)
    49.1 μs ± 4.1 μs, 240 KB allocated, 2.4 KB copied, 8.0 MB peak memory

Map:

  fromAscList:         OK
    39.6 μs ± 2.7 μs, 343 KB allocated, 5.2 KB copied,  10 MB peak memory
  fromAscListWithKey:  OK
    47.0 μs ± 4.3 μs, 455 KB allocated,  12 KB copied,  10 MB peak memory
  fromDescList:        OK
    39.6 μs ± 2.6 μs, 343 KB allocated, 5.5 KB copied,  10 MB peak memory
  fromDescListWithKey: OK
    48.1 μs ± 3.2 μs, 455 KB allocated,  13 KB copied,  10 MB peak memory

@meooow25 meooow25 requested a review from treeowl July 15, 2023 17:57
@meooow25 meooow25 mentioned this pull request Jul 16, 2023
Closed
@meooow25
Copy link
Contributor Author

meooow25 commented Oct 7, 2023

I would like to pick this and related PRs back up and try to get it over the finish line. @treeowl, hope you are well. Would you be available to review these?

@treeowl
Copy link
Contributor

treeowl commented Oct 7, 2023

I will be available to do it on Monday. Please ping me then if you remember.

@meooow25
Copy link
Contributor Author

meooow25 commented Oct 9, 2023

Rebased and updated. @treeowl please take a look.

@meooow25 meooow25 requested a review from treeowl February 9, 2024 13:34
@meooow25
Test and benchmark fromAscList and friends
b4ea836 
Add property tests and benchmarks for fromAscList and related
functions for Set and Map.
@treeowl
Copy link
Contributor

treeowl commented Sep 11, 2024

Why drop Magma? This whole PR slipped under my radar. I'll try to take a look shortly.

treeowl
treeowl reviewed Sep 11, 2024
View reviewed changes
containers-tests/tests/map-properties.hs
t = fromDistinctDescList (reverse nub_sort_xs)
nub_sort_xs = List.map List.head $ List.groupBy ((==) `on` fst) $ List.sortBy (comparing fst) xs
nubDownSortedKxs =
List.map NE.head $
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you didn't do it, but map head q is bletcherous. Why not [x | x : _ <- q] instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's wrong with that? It's NonEmpty's head btw, not list.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah... sorry ....

@meooow25
Copy link
Contributor Author

Why drop Magma?

Switched to arbitrary functions, makes things simpler.

It had also accumulated a bunch of edits, so I squashed a whole thing. You can take a fresh look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@treeowl treeowl treeowl requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@meooow25 @treeowl