Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Minor bug fix for Array.Sort: Replace keys.Length with length for FloorLog2PlusOne#16002

Merged
jkotas merged 2 commits intodotnet:masterfrom
nietras:array-sort-fix-wrong-length-depth-limit
Jan 25, 2018
Merged

Minor bug fix for Array.Sort: Replace keys.Length with length for FloorLog2PlusOne#16002
jkotas merged 2 commits intodotnet:masterfrom
nietras:array-sort-fix-wrong-length-depth-limit

Conversation

@nietras
Copy link

@nietras nietras commented Jan 24, 2018

During my work with implementing https://github.com/dotnet/corefx/issues/15329 I found an issue with the C# implementation of Array.Sort.

The wrong length is used for the introspective sort depth limit. It uses the whole arrays length instead of the sub view input.

The actual impact for this is hard to determine, but worst case the depth limit is way too high causing degenerate cases not to fall back to heap sort, when it should. As far as I can tell.

cc: @jkotas

@jkotas
Copy link
Member

jkotas commented Jan 24, 2018

cc @joperezr @AlexGhiondea

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks!

if (length < 2)
return;

IntroSort(keys, left, length + left - 1, 2 * IntrospectiveSortUtilities.FloorLog2PlusOne(keys.Length), comparer);
Copy link
Member

Choose a reason for hiding this comment

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

The same bug seems to be in the non-generic sort version in Array.cs. Could you please fix it there as well?

Copy link
Author

Choose a reason for hiding this comment

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

@jkotas done, hope I've got them all now :)

@jkotas jkotas merged commit b198091 into dotnet:master Jan 25, 2018
@joperezr
Copy link
Member

LGTM. thanks a lot for the help @nietras

swgillespie pushed a commit to swgillespie/coreclr that referenced this pull request Jan 26, 2018
…orLog2PlusOne (dotnet#16002)

* replace keys.Length with length for FloorLog2PlusOne

* fix non-generic sort
@nietras
Copy link
Author

nietras commented Mar 10, 2018

@jkotas is this fix not part of .NET Core 2.1 Preview1?

@nietras
Copy link
Author

nietras commented Mar 10, 2018

@jkotas I should note that the reason I ask is because this does have an impact on the result of the sorting of keys with items (values). Since HeapSort (for IComparable and for basic types where items type is not same as keys type) is then called in cases where it wasn't called before, hence new Span version calls HeapSort where .NET Core preview1 does not since it does not have the fix it seems. And HeapSort has an issue with how identical keys and their values are sorted.

@jkotas
Copy link
Member

jkotas commented Mar 10, 2018

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants