commons-collections icon indicating copy to clipboard operation
commons-collections copied to clipboard

COLLECTIONS-803: prevent duplicate call to convertKey on put for CaseInsensitiveMap

Open Simulant87 opened this issue 2 years ago • 18 comments

Improves the performance by re-using the once converted key again when creating a new entry. Explained in more detail in https://issues.apache.org/jira/browse/COLLECTIONS-803

Simulant87 avatar Feb 03 '22 20:02 Simulant87

Hello @Simulant87 You'll need to add a test case to this PR that shows at least one failing test when run without the main changes.

garydgregory avatar Feb 03 '22 21:02 garydgregory

@garydgregory Thank you for the feedback. I added a test that verifies the convertKey method is only called once, which fails currently on the master branch without the production code changes provided. I kindly request another review. Please let me know if there is anything else missing.

Simulant87 avatar Feb 14 '22 09:02 Simulant87

I may also suggest changing the convertKey method to use key.toString().toLowerCase(Locale.ROOT) to convert the non-null key, instead of the char loop.

Benefits include better performance (measured a 2 times increase on my pc), and this is more fitted for the use case, as per the Character#toLowerCase documentation:

In general, String.toLowerCase() should be used to map characters to lowercase. 
String case mapping methods have several benefits over Character case mapping methods. 
String case mapping methods can perform locale-sensitive mappings, context-sensitive mappings, and 1:M character mappings, whereas the Character case mapping methods cannot

freya022 avatar Feb 24 '22 18:02 freya022

@freya022 thank you for your suggestion. This sounds like another high performance improvement. However it also sounds like the behaviour might slightly change in some edge cases. Something I expect the maintainers want to avoid and could cause a longer discussion. Therefore I would like to keep your suggestion separated from my changes. As my implementation does not cahnge the behaviour of the key conversion, it should be simpler to review and approve. I encourage your to create a ticket in Jira and create a separate pull request for that change.

Simulant87 avatar Feb 24 '22 19:02 Simulant87

Hello @kinow, I saw that you are able to merge in this repository in another recent PR. Would you also like to review my suggested changes in this PR, please?

Simulant87 avatar Feb 24 '22 19:02 Simulant87

Hello @kinow, I saw that you are able to merge in this repository in another recent PR. Would you also like to review my suggested changes in this PR, please?

Sorry, busy with other pull requests & issues :disappointed_relieved: :bow: code looks good from a brief look, but would need more time to really understand the change and confirm it's a good improvement :+1:

kinow avatar Feb 24 '22 20:02 kinow

Codecov Report

Merging #276 (3ac374a) into master (1677dac) will increase coverage by 0.01%. The diff coverage is 94.73%.

@@             Coverage Diff              @@
##             master     #276      +/-   ##
============================================
+ Coverage     85.87%   85.88%   +0.01%     
- Complexity     4676     4681       +5     
============================================
  Files           292      292              
  Lines         13469    13488      +19     
  Branches       1955     1957       +2     
============================================
+ Hits          11566    11584      +18     
  Misses         1326     1326              
- Partials        577      578       +1     
Impacted Files Coverage Δ
...e/commons/collections4/map/CaseInsensitiveMap.java 92.50% <94.73%> (+2.02%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1677dac...3ac374a. Read the comment docs.

codecov-commenter avatar May 10 '22 20:05 codecov-commenter