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

[WASM] new trimmer feature System.TimeZoneInfo.Invariant #111215

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Jan 8, 2025

Motivation: when TryGetLocalTzFile is included it brings dependency to System.IO.File and its dependencies.

  • new trimmer feature System.TimeZoneInfo.Invariant triggered by existing <InvariantTimezone>true</InvariantTimezone>
    • breaking change: it will prevent loading TZ info files from FS
  • new System.TimeZoneInfo.Invariant getter also driven by DOTNET_SYSTEM_TIMEZONE_INVARIANT env variable
  • cleanup ILLink.Substitutions.LegacyJsInterop.xml

Together with dotnet/sdk#45792

<InvariantTimezone>true</InvariantTimezone> is pre-existing feature.

@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-System.Globalization size-reduction Issues impacting final app size primary for size sensitive workloads labels Jan 8, 2025
@pavelsavara pavelsavara added this to the 10.0.0 milestone Jan 8, 2025
@pavelsavara pavelsavara self-assigned this Jan 8, 2025
Copy link
Contributor

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

@tarekgh
Copy link
Member

tarekgh commented Jan 8, 2025

Will it be acceptable to have DateTime.Now report wrong time when the invariant mode is enabled?

@pavelsavara
Copy link
Member Author

Will it be acceptable to have DateTime.Now report wrong time when the invariant mode is enabled?

It will report UTC. Also this behavior already existed before this PR, here I'm just removing the ability to load TZ data from file system, when InvariantTimezone is true.

@pavelsavara pavelsavara requested a review from ilonatommy January 8, 2025 22:12
@tarekgh
Copy link
Member

tarekgh commented Jan 8, 2025

It will report UTC. Also this behavior already existed before this PR, here I'm just removing the ability to load TZ data from file system, when InvariantTimezone is true.

Thanks. I am not sure my question is answered :-) I am not questioning the PR more than questioning behavior. I guess users haven't run into this yet to report it. Returning UTC time when having TZ settings different than UTC is questionable. Maybe it is acceptable to the browser (especially servers) but wanted to know how we validated this assumption.
Maybe the explicit opt-in switch introduced here is better as users will explicitly enable and understand the consequences.

@pavelsavara
Copy link
Member Author

Returning UTC time when having TZ settings different than UTC is questionable.

This is not how it works. You would not be able to change to non-UTC timezone. Let me show you:

given code

Console.WriteLine($"TimeZoneInfo.Local is {TimeZoneInfo.Local}");
Console.WriteLine($"DateTime.UtcNow is {DateTime.UtcNow}");
Console.WriteLine($"DateTime.Now is {DateTime.Now}");
var start = DateTime.UtcNow;
var timezonesCount = TimeZoneInfo.GetSystemTimeZones().Count;
await Delay(100);
var end = DateTime.UtcNow;
Console.WriteLine($"Found {timezonesCount} timezones in the TZ database in {end - start}");

TimeZoneInfo utc = TimeZoneInfo.FindSystemTimeZoneById("UTC");
Console.WriteLine($"{utc.DisplayName} BaseUtcOffset is {utc.BaseUtcOffset}");

try
{
    TimeZoneInfo tst = TimeZoneInfo.FindSystemTimeZoneById("Asia/Tokyo");
    Console.WriteLine($"{tst.DisplayName} BaseUtcOffset is {tst.BaseUtcOffset}");
}
catch (TimeZoneNotFoundException tznfe)
{
    Console.WriteLine($"Could not find Asia/Tokyo: {tznfe.Message}");
}

At 12:19 in Prague, you will get following output

TimeZoneInfo.Local is (UTC) UTC
DateTime.UtcNow is 1/9/2025 11:19:52 AM
DateTime.Now is 1/9/2025 11:19:52 AM
Found 0 timezones in the TZ database in 00:00:00.1200000
(UTC) UTC BaseUtcOffset is 00:00:00
Could not find Asia/Tokyo: The time zone ID 'Asia/Tokyo' was not found on the local computer.

@pavelsavara
Copy link
Member Author

The <InvariantTimezone>true</InvariantTimezone> feature is opt-in and it means: "I don't care for timezones, please make the application as small as possible".

On the browser "operating system" we don't have TZ database pre-installed and so we are bundling it with each application. It's 300KB of download and some CPU time before the runtime can start. That's a lot for a web app, especially if your business logic is not about dates/times/calendars.

Before this PR the <InvariantTimezone>true</InvariantTimezone> would just stop shipping the data. But you would be able to ship your own and store it into emscripten virtual file system yourself. There are probably nobody in the world that knew this was possible.

I feel quite safe to make this change for the browser target.

@steveisok is this how it works for iOS too ?

Can I assume people are not shipping custom TZDIR there at the same time when they set <InvariantTimezone>true</InvariantTimezone> ?

@pavelsavara pavelsavara marked this pull request as ready for review January 9, 2025 13:16
@pavelsavara pavelsavara requested review from steveisok and removed request for steveisok January 9, 2025 13:16
@steveisok
Copy link
Member

@steveisok is this how it works for iOS too ?

We don't ship anything custom on iOS or Android as we rely on whatever they do.

@pavelsavara pavelsavara force-pushed the browser_trim_timezones branch from b5ce502 to 48ccc77 Compare January 9, 2025 17:37
@pavelsavara pavelsavara changed the title [browser] new trimmer feature System.TimeZoneInfo.Invariant [WASM] new trimmer feature System.TimeZoneInfo.Invariant Jan 9, 2025
}
```

3. setting environment variable value `DOTNET_SYSTEM_TIMEZONE_INVARIANT` to `true` or `1`.
Copy link
Member

Choose a reason for hiding this comment

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

What's the expected precedence if someone sets <InvariantTimezone>true</InvariantTimezone> and DOTNET_SYSTEM_TIMEZONE_INVARIANT=false?

Copy link
Member

Choose a reason for hiding this comment

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

AppContextConfigHelper.GetBooleanConfig (which Invariant Globalization uses) checks the env var first, and then the runtimeconfig:

internal static bool GetBooleanConfig(string switchName, string envVariable, bool defaultValue = false)
{
string? str = Environment.GetEnvironmentVariable(envVariable);
if (str != null)
{
if (str == "1" || bool.IsTrueStringIgnoreCase(str))
{
return true;
}
if (str == "0" || bool.IsFalseStringIgnoreCase(str))
{
return false;
}
}
return GetBooleanConfig(switchName, defaultValue);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

<InvariantTimezone>true</InvariantTimezone> will trigger linker to substitute the call to GetBooleanConfig to constant true.

Copy link
Member

Choose a reason for hiding this comment

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

So with trimming, <InvariantTimezone>true</InvariantTimezone> wins, and without trimming, DOTNET_SYSTEM_TIMEZONE_INVARIANT=false wins. Is that the behavior we want? Typically we try to avoid behavior changes introduced by trimming.

@@ -48,6 +48,7 @@ public TrimmingArgumentBuilder (TestCaseMetadataProvider metadataProvider)
Options.FeatureSwitches.Add ("System.Text.Encoding.EnableUnsafeUTF7Encoding", false);
Options.FeatureSwitches.Add ("System.Diagnostics.Tracing.EventSource.IsSupported", false);
Options.FeatureSwitches.Add ("System.Globalization.Invariant", true);
Options.FeatureSwitches.Add ("System.TimeZoneInfo.Invariant", true);
Copy link
Member

Choose a reason for hiding this comment

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

This is not necessary, we only put here whatever switches are enabled by default in an empty console app.

@@ -40,6 +40,7 @@
<ReproResponseLines Include="--feature:System.Linq.Expressions.CanEmitObjectArrayDelegate=false" />
<ReproResponseLines Include="--feature:System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported=false" />
<ReproResponseLines Include="--feature:System.Globalization.Invariant=true" />
<ReproResponseLines Include="--feature:System.TimeZoneInfo.Invariant=true" />
Copy link
Member

Choose a reason for hiding this comment

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

Same here, not necessary.

@MichalStrehovsky
Copy link
Member

Should this switch be respected on other platforms too? I know it will not lead to real size savings, but consistency is quite important. Feature switches are problematic in general because they change behavior of code that was unit tested or used elsewhere without the feature switch being set: there might be subtle bugs introduced by the feature switch that bypassed quality gates. Making the feature switch effect as visible as possible helps to at least somewhat mitigate that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-wasm WebAssembly architecture area-System.Globalization size-reduction Issues impacting final app size primary for size sensitive workloads
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants