Convert System.Globalization.Native to C #22378

filipnavara · 2019-02-03T01:21:31Z

Revival of #18614.

The original PR was closed and never finished. These were the remaining issues raised during the review:

Lack of use of ICU error handling ( Convert System.Globalization.Native to C #18614 (comment), http://userguide.icu-project.org/design#TOC-Error-Handling)
Inappropriate conversion of C++ enum to C enum ( Convert System.Globalization.Native to C #18614 (comment))
Missing optimization for allocation inside loop ( Convert System.Globalization.Native to C #18614 (comment))

I started to address them. The error handling is not always as pretty as it should be. Notably ucal_set doesn't handle NULL value in first parameter, so explicit error handling must precede it.

Convert u_charsToUChars_safe to ICU style error handling Unify error handling code across functions to use the same patterns Add static modifier to functions that are used only from single C file

Move SortHandle out of public API header, it is opaque structure

filipnavara · 2019-02-03T10:59:38Z

@dotnet-bot test CentOS7.1 x64 Debug Innerloop Build please

filipnavara · 2019-02-03T11:08:01Z

I believe I have addressed all the original feedback and it should be ready for review. Some enum names are not perfect, but it should not be a regression compared to the previous C++ code. Error handling could likely be further refactored and improved, but again, it should not be a regression in the current state.

/cc @jkotas @tarekgh

jkotas

Looks great. Thank you!

am11 · 2019-02-03T14:12:13Z

src/corefx/System.Globalization.Native/pal_icushim.c

                {
-                    return false;
+                    return FALSE;


C99 <stdbool.h> is used in multiple places in CoreFX ref, could it be used instead to save 3 bytes per instance, per call?

How does it actually save anything? In nearly all the instances it ends up in the registers which are not byte-sized anyway.

I removed usage of <stdbool.h> because it was mixed with the ICU's own UBool type and TRUE/FALSE values. There was really no need to mix two or three different boolean types.

Ok. true/false is unsigned char (integral constant macro in smallest range) in the std header, so I always tend to use it over TRUE/FALSE in Unix (and Windows, since VS2013). :)

tarekgh · 2019-02-03T18:39:47Z

src/corefx/System.Globalization.Native/pal_calendarData.c

-        callback(symbolBuf.data(), context);
+        if (symbolBuf != stackSymbolBuf)
+        {
+            free(symbolBuf);


free [](start = 12, length = 4)

would it be better to use the allocated buffer through the loop and then free only once after the loop done or when you need to reallocate?

this is just suggestion but you don't need to to do it if you think this will complicate the code especially I am expecting most of the symbols will fit on the stack buffer

Most, if not all, of the things really fit into the stack buffer, so I don't expect the allocation path to be hit. As such it didn't make much sense to optimize it.

tarekgh · 2019-02-03T18:51:04Z

CC @janvorli

janvorli · 2019-02-04T10:14:07Z

src/corefx/System.Globalization.Native/pal_collation.c

        }
        else
        {
-            pCollator = entry->second;
+            free(map);
+            pCollator = (*(TCollatorMap**)entry)->UCollator;
        }

        pthread_mutex_unlock(&pSortHandle->collatorsLockObject);


It seems the free(map) should be done after the if / else so that it is freed for both code paths.

The whole collatorsPerOption in SortHandle is unused too. I will go through the code and submit a PR to fix it. Thanks for noticing it.

The tsearch adds it as a new node to a binary tree (tfind few lines above guarantees that's the case), so it is not a memory leak. The code unnecessarily does two lookups though. The memory is eventually reclaimed in GlobalizationNative_CloseSortHandle.

Ah, ok. Thank you for looking into it. I haven't realized the tsearch adds the new node.

Allocate initial array using malloc to avoid needlessly zeroing it Reinstate optimization for empty array returned from GetCustomRules lost in dotnet#22378

…tions and GetCollatorFromSortHandle (#24100) * Fix allocation size calculation when resizing array Allocate initial array using malloc to avoid needlessly zeroing it Reinstate optimization for empty array returned from GetCustomRules lost in #22378 * Avoid resizing arrays in GetCustomRules by preallocating the maximum size we can consume (5648 bytes) * Avoid creating a binary search tree for something that could be easily stored as 32-entry lookup table * Remove obsolete comment

Convert System.Globalization.Native to C Commit migrated from dotnet/coreclr@a51af08

…tions and GetCollatorFromSortHandle (dotnet/coreclr#24100) * Fix allocation size calculation when resizing array Allocate initial array using malloc to avoid needlessly zeroing it Reinstate optimization for empty array returned from GetCustomRules lost in dotnet/coreclr#22378 * Avoid resizing arrays in GetCustomRules by preallocating the maximum size we can consume (5648 bytes) * Avoid creating a binary search tree for something that could be easily stored as 32-entry lookup table * Remove obsolete comment Commit migrated from dotnet/coreclr@a59ead8

filipnavara force-pushed the native_cpp_to_C branch 2 times, most recently from f3eda5e to f82860a Compare February 3, 2019 08:00

kasper3 and others added 14 commits February 3, 2019 11:17

Rename files

7d97abd

Convert System.Globalization.Native to C

63d5bab

Address feedback

35326c6

Convert version.cpp to C

db8dbe8

Address feedback

6cd0d92

Fix enum alignment

08bc2bf

Change NormalizationForm to address feedback.

39c5ffd

Simplify and fix error handling

a7f1365

Update one more place to use version.c instead of version.cpp

bea81cd

Fix error handling in GetLocaleIso639LanguageTwoLetterName

1edbceb

Avoid memory allocation in EnumSymbols for small buffers

91df8d2

Update static_assert usage to c_static_assert_msg

5875f89

Fix inconsistent usage of various bool types

c152ba9

Convert u_charsToUChars_safe to ICU style error handling Unify error handling code across functions to use the same patterns Add static modifier to functions that are used only from single C file

Prefix enums, remove DLLEXPORT from GetResultCode

772b2da

Move SortHandle out of public API header, it is opaque structure

filipnavara force-pushed the native_cpp_to_C branch from 4fa7f35 to 772b2da Compare February 3, 2019 10:18

filipnavara changed the title ~~WIP: Convert System.Globalization.Native to C~~ Convert System.Globalization.Native to C Feb 3, 2019

jkotas approved these changes Feb 3, 2019

View reviewed changes

jkotas requested a review from tarekgh February 3, 2019 13:58

am11 reviewed Feb 3, 2019

View reviewed changes

tarekgh reviewed Feb 3, 2019

View reviewed changes

tarekgh approved these changes Feb 3, 2019

View reviewed changes

jkotas merged commit a51af08 into dotnet:master Feb 4, 2019

filipnavara deleted the native_cpp_to_C branch February 4, 2019 08:06

janvorli reviewed Feb 4, 2019

View reviewed changes

filipnavara mentioned this pull request Feb 4, 2019

Avoid duplicate tree lookup in GetCollatorFromSortHandle #22390

Merged

This was referenced Apr 18, 2019

Proxy scenario perf regression dotnet/aspnetcore#9404

Closed

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

Closed

filipnavara mentioned this pull request Apr 18, 2019

[System.Globalization.Native] Fix small issues in CloneCollatorWithOptions and GetCollatorFromSortHandle #24100

Merged

stephentoub mentioned this pull request Apr 22, 2020

Fix weightless code point handling in IndexOf and friends, plus other CompareInfo improvements dotnet/runtime#1514

Merged

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/coreclr#22378 from filipnavara/native_cpp_to_C

5285026

Convert System.Globalization.Native to C Commit migrated from dotnet/coreclr@a51af08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Convert System.Globalization.Native to C #22378

Convert System.Globalization.Native to C #22378

Uh oh!

filipnavara commented Feb 3, 2019 •

edited

Loading

Uh oh!

filipnavara commented Feb 3, 2019

Uh oh!

filipnavara commented Feb 3, 2019

Uh oh!

jkotas left a comment

Uh oh!

am11 Feb 3, 2019

Uh oh!

filipnavara Feb 3, 2019

Uh oh!

filipnavara Feb 3, 2019

Uh oh!

am11 Feb 3, 2019

Uh oh!

tarekgh Feb 3, 2019

Uh oh!

filipnavara Feb 3, 2019 •

edited

Loading

Uh oh!

tarekgh commented Feb 3, 2019

Uh oh!

janvorli Feb 4, 2019

Uh oh!

filipnavara Feb 4, 2019

Uh oh!

filipnavara Feb 4, 2019 •

edited

Loading

Uh oh!

janvorli Feb 4, 2019

Uh oh!

Uh oh!

Convert System.Globalization.Native to C #22378

Convert System.Globalization.Native to C #22378

Uh oh!

Conversation

filipnavara commented Feb 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filipnavara commented Feb 3, 2019

Uh oh!

filipnavara commented Feb 3, 2019

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filipnavara Feb 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekgh commented Feb 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filipnavara Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

filipnavara commented Feb 3, 2019 •

edited

Loading

filipnavara Feb 3, 2019 •

edited

Loading

filipnavara Feb 4, 2019 •

edited

Loading