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 2 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 2 commits into from

Conversation

meooow25
Copy link
Contributor

@meooow25 meooow25 commented Jul 14, 2023 •
edited

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

treeowl
treeowl reviewed Jul 14, 2023
View reviewed changes
containers-tests/tests/map-properties.hs
Copy link
Contributor

Choose a reason for hiding this comment

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

Some explanations are needed for these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which ones are you referring to?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see a bunch of new code. I don't know what it does/what it tests. What's all that gunk with head do? Is there a cleaner way to express whatever that is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests match the current implementations against the equivalent sequence we would get if we did the job using List.groupBy. The only usage of head is on lists out of groupBy (which are known to be non-empty).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with an explanatory comment and switched to NonEmpty.groupBy and NonEmpty.head. Is this better?

@meooow25 meooow25 requested a review from treeowl July 15, 2023 17:57
@meooow25 meooow25 mentioned this pull request Jul 16, 2023
Draft
@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.

treeowl
treeowl requested changes Oct 13, 2023
View reviewed changes
containers-tests/tests/map-properties.hs

prop_list :: [Int] -> Bool
prop_list xs = (sort (nub xs) == [x | (x,()) <- toList (fromList [(x,()) | x <- xs])])

prop_descList :: [Int] -> Bool
prop_descList xs = (reverse (sort (nub xs)) == [x | (x,()) <- toDescList (fromList [(x,()) | x <- xs])])

prop_fromDescList :: [(Int, A)] -> Property
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not at all clear to me what this is testing. Could you explain? Is there a simpler way to express the idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tests that given a list in decreasing order (down_sort_xs), the sequence we get from fromDescList is the same as the reverse of the sequence we would get if we group by the key and take the last key-value pair in each group.

I wouldn't mind changing the code if there is a simpler way to express this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mostly need comments to explain what is being tested. Also: what's with the ()s above? Those look mysterious.

Copy link
Contributor Author

@meooow25 meooow25 Oct 13, 2023 •
edited

Choose a reason for hiding this comment

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

Added some comments.

Also: what's with the ()s above?

I haven't introduced those tests. It seems they want to test that toList . fromList sorts and nubs the keys, and they just use () for the values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, sorry, misread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it better with the comments?

@meooow25 meooow25 requested a review from treeowl February 9, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@treeowl treeowl Awaiting requested review from treeowl

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@meooow25 @treeowl