LilithHafner / InterLanguageSortingComparisons

6 stars 2 forks source link

Optimize Rust comparison function (20% faster overall) #6

Closed exrok closed 1 year ago

exrok commented 1 year ago

Don't know 100% if we want to this. Kind of relying on internal details of the sorting implementation here. However, since this a sorting benchmark I think it makes sense as then the benchmark is focusing of the sorting algorithm.

If the sorting benchmark used something with complete ordering like u64s this wouldn't be a problem.

The perf boost is from avoiding the unwrap which does an extra check for NaNs.

2 Observations:

  1. Internally sort_unstable_by just calls: sort::quicksort(self, |a, b| compare(a, b) == Ordering::Less); So we really only need to check for less than.

  2. Since we have constructed the floats above we don't have to worry about NaNs.

LilithHafner commented 1 year ago

Thank you. This is a reasonable optimization idea that is applicable to all or most of the languages tested here. It partially bypasses the fact that we are sorting IEEE floating point numbers rather than integers. You can fully bypass the fact that we are working with floats by using the unsafe std::mem::transmute because positive finite floats and integers compare the same way.

However, the goal of this benchmark is to test the speed of sorting arbitrary 64-bit IEEE floating point numbers. While the benchmark happens not to test NaN inputs, I would call it cheating to get the wrong answer and/or undefined behavior for those cases. If I recall correctly other languages' implementations do handle NaNs safely (they kind of have to, you wouldn't want sort([2.0,1.0,NaN,4.0]) to fail for a language's default sorting implementation)

Additionally, the primary purpose of this repo is to test the default sorting speed of various languages. If a non-default option (like this) is significantly faster it is a candidate for inclusion but would not replace the default rust sorting speed. (see, for example python and python+numpy)

exrok commented 1 year ago

Seems reasonable, will close for now.