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

[System.Globalization.Native] Fix regressions from #22378 and #22390 #24085

Closed
wants to merge 3 commits into from
Closed

[System.Globalization.Native] Fix regressions from #22378 and #22390 #24085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0bba402
Minor performance improvements for System.Globalization.Native
filipnavara Apr 18, 2019
9f611bf
Add guards against integer overflows, fix one allocation size and lim…
filipnavara Apr 18, 2019
0ee0cd6
Fix performance regression from #22390 on Linux
filipnavara Apr 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/corefx/System.Globalization.Native/pal_alloc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#pragma once

#include <stdlib.h>
#include <stdint.h>

static void *pal_allocarray(size_t numMembers, size_t count)
{
if (numMembers == 0 || count == 0)
return NULL;
if (SIZE_MAX / numMembers < count)
return NULL;
return malloc(numMembers * count);
}

static void *pal_reallocarray(void *ptr, size_t numMembers, size_t count)
{
if (numMembers == 0 || count == 0)
return NULL;
if (SIZE_MAX / numMembers < count)
return NULL;
return realloc(ptr, numMembers * count);
}
10 changes: 7 additions & 3 deletions src/corefx/System.Globalization.Native/pal_calendarData.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <string.h>
#include <strings.h>

#include "pal_alloc.h"
#include "pal_calendarData.h"

#define GREGORIAN_NAME "gregorian"
Expand Down Expand Up @@ -224,14 +225,15 @@ static int InvokeCallbackForDatePattern(const char* locale,
UErrorCode ignore = U_ZERO_ERROR;
int32_t patternLen = udat_toPattern(pFormat, FALSE, NULL, 0, &ignore) + 1;

UChar* pattern = calloc(patternLen, sizeof(UChar));
UChar* pattern = pal_allocarray(patternLen, sizeof(UChar));
if (pattern == NULL)
{
udat_close(pFormat);
return FALSE;
}

udat_toPattern(pFormat, FALSE, pattern, patternLen, &err);
pattern[patternLen - 1] = 0;
udat_close(pFormat);

if (U_SUCCESS(err))
Expand Down Expand Up @@ -264,14 +266,15 @@ static int InvokeCallbackForDateTimePattern(const char* locale,
UErrorCode ignore = U_ZERO_ERROR;
int32_t patternLen = udatpg_getBestPattern(pGenerator, patternSkeleton, -1, NULL, 0, &ignore) + 1;

UChar* bestPattern = calloc(patternLen, sizeof(UChar));
UChar* bestPattern = pal_allocarray(patternLen, sizeof(UChar));
if (bestPattern == NULL)
{
udatpg_close(pGenerator);
return FALSE;
}

udatpg_getBestPattern(pGenerator, patternSkeleton, -1, bestPattern, patternLen, &err);
bestPattern[patternLen - 1] = 0;
udatpg_close(pGenerator);

if (U_SUCCESS(err))
Expand Down Expand Up @@ -332,7 +335,7 @@ static int32_t EnumSymbols(const char* locale,
}
else
{
symbolBuf = calloc(symbolLen, sizeof(UChar));
symbolBuf = pal_allocarray(symbolLen, sizeof(UChar));
if (symbolBuf == NULL)
{
err = U_MEMORY_ALLOCATION_ERROR;
Expand All @@ -341,6 +344,7 @@ static int32_t EnumSymbols(const char* locale,
}

udat_getSymbols(pFormat, type, i, symbolBuf, symbolLen, &err);
symbolBuf[symbolLen - 1] = 0;

if (U_SUCCESS(err))
{
Expand Down
29 changes: 19 additions & 10 deletions src/corefx/System.Globalization.Native/pal_collation.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <search.h>
#include <string.h>

#include "pal_alloc.h"
#include "pal_collation.h"

c_static_assert_msg(UCOL_EQUAL == 0, "managed side requires 0 for equal strings");
Expand Down Expand Up @@ -142,8 +143,12 @@ static int AddItem(UCharList* list, const UChar item)
size_t size = list->size++;
if (size >= list->capacity)
{
list->capacity *= 2;
UChar* ptr = (UChar*)realloc(list->items, list->capacity * sizeof(UChar*));
list->capacity += 512;
if (list->capacity < size)
Copy link
Member

@stephentoub stephentoub Apr 18, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@filipnavara filipnavara Apr 18, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@filipnavara filipnavara Apr 18, 2019

Choose a reason for hiding this comment

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

I'll revert it to *2 since it doesn't really matter.

{
return FALSE;
}
UChar* ptr = (UChar*)pal_reallocarray(list->items, list->capacity, sizeof(UChar));
if (ptr == NULL)
{
return FALSE;
Expand Down Expand Up @@ -188,15 +193,14 @@ static UCharList* GetCustomRules(int32_t options, UColAttributeValue strength, i
// Use 512 as the starting size, so the customRules won't have to grow if we are just
// doing the KanaType custom rule.
customRules->capacity = 512;
customRules->items = calloc(customRules->capacity, sizeof(UChar));
customRules->size = 0;
customRules->items = pal_allocarray(customRules->capacity, sizeof(UChar));
if (customRules->items == NULL)
{
free(customRules);
return NULL;
}

customRules->size = 0;

if (needsIgnoreKanaTypeCustomRule || needsNotIgnoreKanaTypeCustomRule)
{
UChar compareChar = needsIgnoreKanaTypeCustomRule ? '=' : '<';
Expand Down Expand Up @@ -280,7 +284,7 @@ UCollator* CloneCollatorWithOptions(const UCollator* pCollator, int32_t options,

UCollator* pClonedCollator;
UCharList* customRules = GetCustomRules(options, strength, isIgnoreSymbols);
if (customRules == NULL)
if (customRules == NULL || customRules->size == 0)
{
pClonedCollator = ucol_safeClone(pCollator, NULL, NULL, pErr);
}
Expand All @@ -292,7 +296,7 @@ UCollator* CloneCollatorWithOptions(const UCollator* pCollator, int32_t options,
const UChar* localeRules = ucol_getRules(pCollator, &localeRulesLength);
int32_t completeRulesLength = localeRulesLength + customRuleLength + 1;

UChar* completeRules = calloc(completeRulesLength, sizeof(UChar));
UChar* completeRules = pal_allocarray(completeRulesLength, sizeof(UChar));

for (int i = 0; i < localeRulesLength; i++)
{
Expand All @@ -302,11 +306,12 @@ UCollator* CloneCollatorWithOptions(const UCollator* pCollator, int32_t options,
{
completeRules[localeRulesLength + i] = customRules->items[i];
}
completeRules[completeRulesLength - 1] = 0;

pClonedCollator = ucol_openRules(completeRules, completeRulesLength, UCOL_DEFAULT, strength, NULL, pErr);
free(completeRules);
free(customRules);
}
free(customRules);

if (isIgnoreSymbols)
{
Expand Down Expand Up @@ -439,11 +444,15 @@ const UCollator* GetCollatorFromSortHandle(SortHandle* pSortHandle, int32_t opti

TCollatorMap* map = (TCollatorMap*)malloc(sizeof(TCollatorMap));
map->key = options;
void* entry = tsearch(map, &pSortHandle->collatorsPerOptionRoot, TreeComparer);
if ((*(TCollatorMap**)entry) == map)
// tfind on glibc is significantly faster than tsearch and we expect
// to hit the cache here often so it's benefitial to prefer lookup time
// over addition time
void* entry = tfind(map, &pSortHandle->collatorsPerOptionRoot, TreeComparer);
if (entry == NULL)
{
pCollator = CloneCollatorWithOptions(pSortHandle->regular, options, pErr);
map->UCollator = pCollator;
tsearch(map, &pSortHandle->collatorsPerOptionRoot, TreeComparer);
Copy link
Member

@stephentoub stephentoub Apr 18, 2019

Choose a reason for hiding this comment

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

Can we separate out the regression fix into its own PR?

Copy link
Member Author

@filipnavara filipnavara Apr 18, 2019

Choose a reason for hiding this comment

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

Sure

}
else
{
Expand Down
10 changes: 7 additions & 3 deletions src/corefx/System.Globalization.Native/pal_localeNumberData.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <stdlib.h>
#include <string.h>

#include "pal_alloc.h"
#include "pal_localeNumberData.h"

// invariant character definitions used by ICU
Expand Down Expand Up @@ -86,13 +87,13 @@ static char* NormalizeNumericPattern(const UChar* srcPattern, int isNegative)
if (isNegative && !minusAdded)
{
size_t length = (iEnd - iStart) + 2;
destPattern = calloc(length, sizeof(char));
destPattern = pal_allocarray(length, sizeof(char));
destPattern[index++] = '-';
}
else
{
size_t length = (iEnd - iStart) + 1;
destPattern = calloc(length, sizeof(char));
destPattern = pal_allocarray(length, sizeof(char));
}

for (int i = iStart; i <= iEnd; i++)
Expand Down Expand Up @@ -142,6 +143,8 @@ static char* NormalizeNumericPattern(const UChar* srcPattern, int isNegative)
}
}

destPattern[index] = 0;

return destPattern;
}

Expand All @@ -164,7 +167,7 @@ static int GetNumericPattern(const UNumberFormat* pNumberFormat,
UErrorCode ignore = U_ZERO_ERROR;
int32_t icuPatternLength = unum_toPattern(pNumberFormat, FALSE, NULL, 0, &ignore) + 1;

UChar* icuPattern = calloc(icuPatternLength, sizeof(UChar));
UChar* icuPattern = pal_allocarray(icuPatternLength, sizeof(UChar));
if (icuPattern == NULL)
{
return U_MEMORY_ALLOCATION_ERROR;
Expand All @@ -173,6 +176,7 @@ static int GetNumericPattern(const UNumberFormat* pNumberFormat,
UErrorCode err = U_ZERO_ERROR;

unum_toPattern(pNumberFormat, FALSE, icuPattern, icuPatternLength, &err);
icuPattern[icuPatternLength - 1] = 0;

assert(U_SUCCESS(err));

Expand Down
7 changes: 5 additions & 2 deletions src/corefx/System.Globalization.Native/pal_localeStringData.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <stdlib.h>
#include <string.h>

#include "pal_alloc.h"
#include "pal_localeStringData.h"

/*
Expand Down Expand Up @@ -77,13 +78,14 @@ static UErrorCode GetLocaleIso639LanguageTwoLetterName(const char* locale, UChar
UErrorCode status = U_ZERO_ERROR, ignore = U_ZERO_ERROR;
int32_t length = uloc_getLanguage(locale, NULL, 0, &ignore) + 1;

char* buf = calloc(length, sizeof(char));
char* buf = pal_allocarray(length, sizeof(char));
if (buf == NULL)
{
return U_MEMORY_ALLOCATION_ERROR;
}

uloc_getLanguage(locale, buf, length, &status);
buf[length - 1] = 0;
u_charsToUChars_safe(buf, value, valueLength, &status);
free(buf);

Expand Down Expand Up @@ -120,13 +122,14 @@ static UErrorCode GetLocaleIso3166CountryName(const char* locale, UChar* value,
UErrorCode status = U_ZERO_ERROR, ignore = U_ZERO_ERROR;
int32_t length = uloc_getCountry(locale, NULL, 0, &ignore) + 1;

char* buf = calloc(length, sizeof(char));
char* buf = pal_allocarray(length, sizeof(char));
if (buf == NULL)
{
return U_MEMORY_ALLOCATION_ERROR;
}

uloc_getCountry(locale, buf, length, &status);
buf[length - 1] = 0;
u_charsToUChars_safe(buf, value, valueLength, &status);
free(buf);

Expand Down