[System.Globalization.Native] Fix regressions from #22378 and #22390 #24085
Conversation
| int32_t patternLen = udat_toPattern(pFormat, FALSE, NULL, 0, &ignore) + 1; | ||
|
|
||
| UChar* pattern = calloc(patternLen, sizeof(UChar)); | ||
| UChar* pattern = malloc(patternLen * sizeof(UChar)); |
There was a problem hiding this comment.
Why is this an improvement? Don't we then need to be concerned about overflow and potential resulting buffer overflows?
There was a problem hiding this comment.
Because calloc zeroes the allocated memory. In all the cases that was actually redundant because it was overwritten with valid data just few lines after the allocation. If overflow in the multiplication is valid concern I can wrap that up in a macro or helper function and check for it.
There was a problem hiding this comment.
In CoreFX, add_s and multiply_s (https://github.com/dotnet/corefx/blob/0fd3b7e/src/Native/Unix/Common/pal_safecrt.h#L17) are used for safe arithmetic before calling malloc. The implementation uses compiler intrinsic and provides software fallback. Maybe pal_safecrt.h can be ported from CoreFX?
There was a problem hiding this comment.
@am11 That makes sense. I was about to write similar helper function, but in the end I decided to write a simple portable code instead. I didn't notice that it already exists in CoreFX.
I actually have an open issue for moving System.Globalization.Native to CoreFX - https://github.com/dotnet/coreclr/issues/22391 - so it makes sense to converge the code.
filipnavara
force-pushed
the
sysglobnative
branch
from
0c17d06 to
5c4218d
Compare
filipnavara
changed the title
…it array resizing to stable increments
filipnavara
force-pushed
the
sysglobnative
branch
from
5c4218d to
9f611bf
Compare
filipnavara
changed the title
filipnavara
force-pushed
the
sysglobnative
branch
from
9984be8 to
0ee0cd6
Compare
| list->capacity *= 2; | ||
| UChar* ptr = (UChar*)realloc(list->items, list->capacity * sizeof(UChar*)); | ||
| list->capacity += 512; | ||
| if (list->capacity < size) |
There was a problem hiding this comment.
The PR is making a handful of unrelated improvements with the goal of improving perf... can you share measurements that demonstrate they move things in a positive direction? I'm not clear on why this change to +512 instead of *2 is definitively better.
There was a problem hiding this comment.
The safety check I added is actually never going to get hit. GetCustomRules has upper bound on how many custom rules can get added, so realistically the only two cases that can happen is the default array size (512) or the next bigger one (1024 both before and after my change).
The maximum size at the moment is something around 860.
There was a problem hiding this comment.
I'll revert it to *2 since it doesn't really matter.
| { | ||
| pCollator = CloneCollatorWithOptions(pSortHandle->regular, options, pErr); | ||
| map->UCollator = pCollator; | ||
| tsearch(map, &pSortHandle->collatorsPerOptionRoot, TreeComparer); |
There was a problem hiding this comment.
Can we separate out the regression fix into its own PR?
3 participants
Address few regressions from #22378. Avoid using
callocin order to remove overhead from unnecessary zeroing of memory.Fix reallocation in
AddItem(pal_collation.c) to prevent accidental overflow and to use correct element size. Previously it allocated more memory due to incorrectly multiplying bysizeof(UChar*)instead ofsizeof(UChar).Fix performance regression from #22390 (tracked in dotnet/aspnetcore#9404).
tsearchperformance on glibc is vastly inferior totfindso prefer to use the later. On macOS the performance of either approach was nearly identical.