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

[release/8.0] Revert performance optimization resulting in incorrect lookups in certain case insensitive frozen collections #94685

Conversation

eiriktsarpalis
Copy link
Member

Backport of #94667 to release/8.0

Customer impact

Addresses a customer reported issue where under certain circumstances, frozen collections produce erroneous lookup results when keyed on case-insensitive strings. After a lot deliberation, we have decided to revert the particular performance optimization that was causing the bug. While this change will result in perf regression, we are fine with this since

  1. It only impacts the use case which is currently misbehaving and
  2. It is the smallest possible change that fixes the bug.

Testing

Added unit testing covering the impacted scenario.

Risk

Low. Makes targeted changes in the KeyAnalyzer component disabling the optimization for the cases where it isn't valid.

@ghost
Copy link

ghost commented Nov 13, 2023

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #94667 to release/8.0

Customer impact

Addresses a customer reported issue where under certain circumstances, frozen collections produce erroneous lookup results when keyed on case-insensitive strings. After a lot deliberation, we have decided to revert the particular performance optimization that was causing the bug. While this change will result in perf regression, we are fine with this since

  1. It only impacts the use case which is currently misbehaving and
  2. It is the smallest possible change that fixes the bug.

Testing

Added unit testing covering the impacted scenario.

Risk

Low. Makes targeted changes in the KeyAnalyzer component disabling the optimization for the cases where it isn't valid.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Collections

Milestone: -

@eiriktsarpalis eiriktsarpalis added this to the 8.0.x milestone Nov 13, 2023
@eiriktsarpalis eiriktsarpalis added the Servicing-consider Issue for next servicing release review label Nov 13, 2023
@carlossanlop
Copy link
Member

Friendly reminder: If you'd like this to be included in the December release, please merge it before Tuesday November 14th EOD (Code Complete).

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

LGTM!

@jeffhandley
Copy link
Member

@carlossanlop Should this target the release/8.0-staging branch instead of release/8.0?

@eiriktsarpalis eiriktsarpalis added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Nov 14, 2023
@eiriktsarpalis
Copy link
Member Author

Approved over email.

@eiriktsarpalis eiriktsarpalis changed the base branch from release/8.0 to release/8.0-staging November 14, 2023 17:45
…ivity in FrozenCollections (dotnet#94667)

* Add failing tests

* Fix incorrect case sensitivity in FrozenDictionary and FrozenSet for some cases

fixes dotnet#93974

* When hashing the entire string, case sensitivity of hash and equals should be the same

* Address code review comments

* Only ignore case insensitivity if entire string is ASCII non-letters

* Code review comments

* Undo some new lines

* Fixed tests - incorrect leftover from previous PR
@eiriktsarpalis eiriktsarpalis force-pushed the backport-frozencollection-ignorecase-fix branch from e72d5e1 to 5fd9852 Compare November 14, 2023 17:50
@eiriktsarpalis
Copy link
Member Author

Build errors are known issues.

@eiriktsarpalis eiriktsarpalis merged commit e585246 into dotnet:release/8.0-staging Nov 14, 2023
105 of 108 checks passed
@carlossanlop carlossanlop modified the milestones: 8.0.x, 8.0.1 Nov 16, 2023
@eiriktsarpalis eiriktsarpalis deleted the backport-frozencollection-ignorecase-fix branch December 5, 2023 11:18
@github-actions github-actions bot locked and limited conversation to collaborators Jan 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Collections Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants