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

Principled Setoid and Ord with improved performance #163

Merged
merged 3 commits into from
Dec 13, 2023
Merged

Conversation

Avaq
Copy link
Member

@Avaq Avaq commented Aug 19, 2021

Opened a draft PR for discussion.
See #151 (comment).

Remember we did that thing where we made Z.equals more useful by having it fall back to structural equality for garbage inputs (#154)? We ended up with a useful, but unprincipled and slow, equality function. That's fine and all, but I don't like that:

  • Sanctuary is now forced to use this slow function, despite already having ensured that inputs are Setoids of the same type.
  • Many Setoid implementations build on Z.equals, making them slow for no reason.
  • Some Ord implementations, and Z.lte itself, builds on Z.equals.

index.js Outdated Show resolved Hide resolved
@Avaq Avaq mentioned this pull request Aug 19, 2021
@Avaq
Copy link
Member Author

Avaq commented Aug 19, 2021

The new code is problematic: Z.equals doesn't check that both inputs are the same Setoid all the way down; it checks only x before dispatching to setoidEquals, meaning it will dispatch with garbage inputs such as equals ([null], [42]) for which setoidEquals will happily return true because of the Setoid implementation for null.

@Avaq
Copy link
Member Author

Avaq commented Aug 19, 2021

This probably means that we'd have to have two completely separate implementations for equals and setoidEquals, with two separate type classes. This is something I hoped to avoid initially.

@Avaq Avaq changed the title Further, but breaking, performance improvements Principled equality with improved performance Aug 19, 2021
@Avaq Avaq force-pushed the avaq/performance branch 3 times, most recently from 07b4ea4 to dd4f87d Compare August 29, 2021 09:09
Base automatically changed from avaq/performance to master August 30, 2021 15:54
@Avaq Avaq force-pushed the avaq/equality branch 2 times, most recently from 6f205a1 to 5646866 Compare August 30, 2021 17:54
index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@Avaq Avaq force-pushed the avaq/equality branch 2 times, most recently from b738613 to 71c9d2a Compare August 31, 2021 07:41
@Avaq
Copy link
Member Author

Avaq commented Feb 26, 2022

Right, so I removed the whole structural deep equality function, which I believe we said (face to face) we should reimplement in a separate package if the need is great enough.

index.js Outdated Show resolved Hide resolved
@Avaq Avaq force-pushed the avaq/equality branch 2 times, most recently from e130a82 to b3d831d Compare February 26, 2022 18:29
@Avaq
Copy link
Member Author

Avaq commented Feb 26, 2022

The effect of these changes on performance:

$ npm run bench -- --benchmark old-vs-new --match '*{equals,lte}*'

> sanctuary-type-classes@13.0.0 bench
> sanctuary-benchmark "--benchmark" "old-vs-new" "--match" "*{equals,lte}*"

Running benchmarks: old-vs-new...

┌─────────────────────────┬──────────────────────────┬──────────────────────────┬────────┬──────────┬───┐                                                                                                                                                      
│ suite                   │ old                      │ new                      │ diff   │ change   │ α │
├─────────────────────────┼──────────────────────────┼──────────────────────────┼────────┼──────────┼───┤
│ methods.equals.Identity │ 202,779 Hz ±0.22% (n 94) │ 209,578 Hz ±0.59% (n 95) │ 001.6% │ +003.4%  │   │
├─────────────────────────┼──────────────────────────┼──────────────────────────┼────────┼──────────┼───┤
│ methods.equals.Object   │ 57,084 Hz ±1.84% (n 87)  │ 395,393 Hz ±2.48% (n 83) │ 074.8% │ +592.7%  │ ✓ │
├─────────────────────────┼──────────────────────────┼──────────────────────────┼────────┼──────────┼───┤
│ methods.lte.Identity    │ 145,642 Hz ±0.68% (n 93) │ 139,259 Hz ±4.06% (n 78) │ 002.2% │ -004.4%  │   │
├─────────────────────────┼──────────────────────────┼──────────────────────────┼────────┼──────────┼───┤
│ methods.lte.Object      │ 37,530 Hz ±2.05% (n 89)  │ 444,342 Hz ±2.20% (n 89) │ 084.4% │ +1084.0% │ ✓ │
└─────────────────────────┴──────────────────────────┴──────────────────────────┴────────┴──────────┴───┘

@Avaq Avaq changed the title Principled equality with improved performance Principled Setoid and Ord with improved performance Feb 26, 2022
@Avaq
Copy link
Member Author

Avaq commented Feb 26, 2022

If we go this route, this will naturally have to be released as a breaking change. I don't expect many people were using lte and friends on anything other than real Ords, but I expect a few users to be impacted by the change to equals.

@Avaq
Copy link
Member Author

Avaq commented Feb 26, 2022

Although.. it hasn't been that long ago that equals was producing real useless garbage output for non-Setoids. Maybe it's not too bad.

@Avaq Avaq marked this pull request as ready for review February 26, 2022 18:40
@Avaq Avaq requested a review from davidchambers February 26, 2022 18:41
@Avaq

This comment was marked as resolved.

test/index.js Outdated Show resolved Hide resolved
test/index.js Show resolved Hide resolved
@Avaq Avaq force-pushed the avaq/equality branch 2 times, most recently from bf817a9 to d1884ff Compare December 13, 2023 13:37
The new GitHub Markdown syntax for highlighted blockquotes is not
compatible with Remark's detection of broken links, and caused false
positives.

Also we will be removing Remark from the project soon anyway.
This has massive performance benefits for the equals function.
Copy link
Member

@davidchambers davidchambers left a comment

Choose a reason for hiding this comment

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

🎉 🎉 🎉

@Avaq Avaq merged commit d83fa7d into main Dec 13, 2023
3 checks passed
@Avaq Avaq deleted the avaq/equality branch December 13, 2023 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants