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

Try loading build time ICU version. #79251

Closed

Conversation

wscho77
Copy link
Contributor

@wscho77 wscho77 commented Dec 5, 2022

Calling dlopen() function multiple times from an unknown max version to find the ICU library makes performance degradation under heavy IO contention.
To avoid this situation, try loading first with the ICU version used at build time.

In our tests, it took up to 100ms under heavy IO contention.

Calling dlopen() function multiple times from an unknown max version to find the ICU library
makes performance degradation under heavy IO contention.
To avoid this situation, try loading first with the ICU version used at build time.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 5, 2022
@ghost
Copy link

ghost commented Dec 5, 2022

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

Issue Details

Calling dlopen() function multiple times from an unknown max version to find the ICU library makes performance degradation under heavy IO contention.
To avoid this situation, try loading first with the ICU version used at build time.

In our tests, it took up to 100ms under heavy IO contention.

Author: wscho77
Assignees: -
Labels:

area-System.Globalization

Milestone: -

@tarekgh
Copy link
Member

tarekgh commented Dec 5, 2022

This is wrong, this will prevent loading latest version if there are two ICU versions on the system. This scenario is possible.

CC @janvorli

@tarekgh tarekgh added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 5, 2022
@tarekgh
Copy link
Member

tarekgh commented Dec 5, 2022

I am closing it, feel free to send any question or thought we can consider. Thanks for trying.

@tarekgh tarekgh closed this Dec 5, 2022
@janvorli
Copy link
Member

janvorli commented Dec 5, 2022

@wscho77 you can solve this by passing in an explicit version of the ICU by setting CLR_ICU_VERSION_OVERRIDE env variable. Then we would use that version as the first attempt.

@wscho77
Copy link
Contributor Author

wscho77 commented Dec 5, 2022

@wscho77 you can solve this by passing in an explicit version of the ICU by setting CLR_ICU_VERSION_OVERRIDE env variable. Then we would use that version as the first attempt.

Thank your comment.
We are also considering about using "CLR_ICU_VERSION_OVERRIDE".
But we are finding alternative method to set build time ICU version because different ICU version can be used for each platform.

@tarekgh @janvorli Would you accept introducing another override env variable that sets to use build time icu version?

@wscho77
Copy link
Contributor Author

wscho77 commented Dec 6, 2022

And shouldn't the environment variable name be changed to "DOTNET_ICU_VERSION_OVERRIDE"?

@ghost ghost locked as resolved and limited conversation to collaborators Jan 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Globalization community-contribution Indicates that the PR has been added by a community member NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants