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

Let Array#sort only use `<=>`, and let `<=>` return `nil` for partial comparability #6611

Merged
merged 5 commits into from Mar 15, 2019

Conversation

@asterite
Copy link
Member

commented Aug 26, 2018

Fixes #6608

This PR does a few things:

  • Array#sort and related methods used to use < and <= instead of <=>, as documented. This is fixed to only use <=>.
  • Merge Comparable and PartialComparable into a single type. <=> can now return nil, meaning that the two objects aren't comparable. This can be used when two objects of the same type can be compared, but not always (partial order). One such example are floats: they are usually comparable, except for NaN values.
  • Let Array#sort and related methods handle the case when <=> return nil, and raise an exception in that case
  • Let Number <=> Number return nil for the case of NaN values, but provide an optimized implementation for Int <=> Int that doesn't return nil

I ran the benchmarks provided in the PR that introduced intro sort, but only checking for sort and sort with block, and compared them to the current implementation. The performance is the same, except when <=> can return nil, in which the sorting is a bit less than 3 times slower. But that makes sense, because there's a nil check for every comparison now. I personally prefer to give a correct result than to silently fail at runtime. But this performance penalty only applies when <=> returns nil, so it's not a very common case.

The reason for removing PartialComparable and merging it with Comparable is that it's simpler to have just one module, and let it automatically indicate partial order if nil is returned. If nil isn't returned, compiling with --release should give the same performance, because the check for nil will be optimized out.

This PR also fixes the fact that even though we had PartialComparable, you couldn't sort such types (Array#sort didn't handle the possibility of nil from <=>).

Note: I replaced an Int32? restriction with U forall U to allow for sort's block to be of type Int32 or Int32?, for performance reasons. Ideally we should be able to do U < Int32? or some other condition over a free variable, but that's currently not supported. Eventually we can improve that code.

l.value = p.value
l, p = p, p - 1
end
l.value = v
end
end

protected def self.cmp(v1, v2)
v = v1 <=> v2
raise ArgumentError.new("comparison of #{v1} and #{v2} failed") if v.nil?

This comment has been minimized.

Copy link
@Sija

Sija Aug 26, 2018

Contributor

In exception messages title-case if preferred. Ditto below.

This comment has been minimized.

Copy link
@asterite

asterite Aug 26, 2018

Author Member

I wish we hand't change that. This is how it looks right now:

Unhandled exception: Index out of bounds (IndexError)
  from src/indexable.cr:596:8 in 'at'
  from src/indexable.cr:73:5 in '[]'
  from baz.cr:2:1 in '__crystal_main'
  from src/crystal/main.cr:97:5 in 'main_user_code'
  from src/crystal/main.cr:86:7 in 'main'
  from src/crystal/main.cr:106:3 in 'main'

It doesn't look good to have an uppercase letter after :. This is Ruby:

baz.cr:1:in `sort': comparison of Integer with 2 failed (ArgumentError)
	from baz.cr:1:in `<main>'

I'll capitalize it, but I'd like to discuss reverting this capitalization.

This comment has been minimized.

Copy link
@asterite

asterite Aug 26, 2018

Author Member

Fixed (rebased)

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Aug 26, 2018

I'd suggest to add the explanation about the performance regarding Nil to the API docs.

@asterite

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2018

@straight-shoota What do you suggest? Something like "You can return nil if your need partial ordering. However, that will result in a slower performance when sorting such objects. In that case, you might considering not modeling your program correctly (as you should) just so that you can gain a small performance improvement that will probably never be the bottleneck of your program"?

@asterite

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2018

@straight-shoota With the above I don't mean to be rude, I just don't understand how we can recommend someone not to model their program correctly because it will perform worse. But I'm open to suggestions.

@asterite asterite force-pushed the asterite:feature/spaceship branch from 1fe21d4 to 9b77b60 Aug 26, 2018

@asterite

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2018

@straight-shoota Or, well, I could add "Note that if there is a chance that<=> returns nil, Array#sort will perform a bit slower". But I'd rather not to, because then many would probably try to prematurely optimize their program at the cost of a worse modeling.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Aug 26, 2018

No offense taken. But I think it should be worth mentioning that there is a difference in performance depending on whether Nil type is included or not. If written positively, like for completely comparable types the comparator value is not nilable and the nil check will be optimized out.
Obviously it is not a good advice to apply an incorrect comparison model just to be faster. But it is good to know that when you have a completely comparable type, that it wont waste performance on nil checks.

This was actually my most important concern when reading the PR (and your comments in #6608), how performance would play out for sorting completely comparable types vs. partially comparable types. And I think the implementation is really great.

@asterite

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2018

@straight-shoota Sounds good. Where should I add such comment? In Comparable, in Comparable#<=> or in one of the Array#sort methods?

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

I think I'd put it in Comparable following the description of nil return value.

@asterite asterite force-pushed the asterite:feature/spaceship branch 3 times, most recently from 999d37e to fa4e9d6 Aug 28, 2018

@asterite

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

@straight-shoota Added the performance note.

Show resolved Hide resolved src/array.cr Outdated
@straight-shoota

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@asterite Could you rebase this?

@asterite asterite force-pushed the asterite:feature/spaceship branch from fa4e9d6 to 3d9737a Feb 11, 2019

@asterite

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

@straight-shoota Done! :-)

Show resolved Hide resolved src/comparable.cr Outdated
@straight-shoota
Copy link
Member

left a comment

Yes, inlined is much better indeed 👍

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

@asterite Needs a rebase!

asterite added some commits Aug 26, 2018

Let Array#sort only use `<=>`, and let `<=>` return `nil` for partial…
… comparability.

- Float <=> Float now will return `nil` for NaN
- Removed PartialComparable

@asterite asterite force-pushed the asterite:feature/spaceship branch from bd311e7 to f01a0e9 Mar 13, 2019

@asterite

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

Rebased!

@asterite

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

There was a recent PR that improved Comparable's docs. In this PR I had already done that so I kept my docs which also includes the fact that nil can be returned now from <=>. I should have mentioned that in the other PR to avoid wasting someone else's time.

Show resolved Hide resolved src/comparable.cr Outdated
Show resolved Hide resolved src/number.cr Outdated
Show resolved Hide resolved src/int.cr Outdated
Show resolved Hide resolved src/compiler/crystal/macros/methods.cr Outdated
Show resolved Hide resolved src/comparable.cr Outdated
Show resolved Hide resolved src/comparable.cr Outdated

@asterite asterite force-pushed the asterite:feature/spaceship branch from b07a102 to 0140f77 Mar 13, 2019

@asterite asterite merged commit b07a845 into crystal-lang:master Mar 15, 2019

4 of 5 checks passed

ci/circleci: test_darwin Your tests failed on CircleCI
Details
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

urde-graven pushed a commit to urde-graven/crystal that referenced this pull request Mar 20, 2019

Urde Graven
Merge branch 'master' of github.com:crystal-lang/crystal
* 'master' of github.com:crystal-lang/crystal:
  Change the font-weight used in Playground (crystal-lang#7552)
  Fix formatting absolute paths (crystal-lang#7560)
  Refactor IO::Syscall as IO::Evented (crystal-lang#7505)
  YAML: fix test checking serialization of slices for libyaml 0.2.2 (crystal-lang#7555)
  Let Array#sort only use `<=>`, and let `<=>` return `nil` for partial comparability (crystal-lang#6611)
  Update `to_s` and `inspect` to have similar signatures accross the stdlib (crystal-lang#7528)
  Fix restriction of valid type vs free vars (crystal-lang#7536)
  Rewrite macro spec without executing a shell command (crystal-lang#6962)
  Suggest `next` when trying to break from captured block  (crystal-lang#7406)
  Fix GenericClassType vs GenericClassInstanceType restrictions (crystal-lang#7537)
  Add human readable formatting for numbers (crystal-lang#6314)
  Add command and args to execvp error message (crystal-lang#7511)
  Implement Set#add? method (crystal-lang#7495)
@r00ster91

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

Shouldn't the src/partial_comparable.cr file now be deleted?

@asterite

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

Yes, but it's a breaking change to do so, so we must be a bit careful.

@r00ster91

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

Unfortunately the Deprecated annotation doesn't support modules yet. I would say it should just be removed because it's not really used anywhere: https://github.com/search?l=Crystal&q=PartialComparable&type=Code.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

Why don't we add a deprecation notice in the API docs for 0.28.0 and remove it in 0.29.0?

@bcardiff

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

We could flag the <=> method as deprecated during 0.28 if someone wants to tackle that.
The std lib path is not ignored by default, so that should warn about PartialComparable usages.

Maybe another check for deprecated methods is to detect overrides of abstract methods with @[Deprecated].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.