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

Add feature-switch for invariant TimeZone support #37973

Closed
marek-safar opened this issue Jun 16, 2020 · 13 comments · Fixed by #87284
Closed

Add feature-switch for invariant TimeZone support #37973

marek-safar opened this issue Jun 16, 2020 · 13 comments · Fixed by #87284
Assignees
Labels
arch-wasm WebAssembly architecture area-System.DateTime size-reduction Issues impacting final app size primary for size sensitive workloads
Milestone

Comments

@marek-safar
Copy link
Contributor

For size sensitive apps it'd be very helpful to have an option (feature switch) to exclude full time-zones support. This is something that is supported by Blazor 3.2 as an opt-in feature which we should not regress on.

@eerhardt

@marek-safar marek-safar added the linkable-framework Issues associated with delivering a linker friendly framework label Jun 16, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Infrastructure-installer untriaged New issue has not been triaged by the area owner labels Jun 16, 2020
@Clockwork-Muse
Copy link
Contributor

... This is implying that we'd be distributing our own copy of timezone data? Or is this something else? Traditionally, we've pulled from the OS for desktop/server deployments, which would mean the size of the timezone data with an app is 0.

Note that the full tzdb is < 400kb, and is trivially amenable to CDN delivery/caching. Perhaps Blazor should also add the ability to specify a web resource for this, so it can be delivered out-of-band from the app itself?

@joperezr joperezr removed the untriaged New issue has not been triaged by the area owner label Jul 7, 2020
@joperezr joperezr added this to the 5.0.0 milestone Jul 7, 2020
@joperezr
Copy link
Member

joperezr commented Jul 7, 2020

@eerhardt at least the setting seems to already be present in the SDK so isn't this one done already? https://github.com/dotnet/sdk/blob/408fe82020f979ec65862add4d5b1dd85fef5d80/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets#L396-L399

What am I missing?

@Clockwork-Muse The point here is to have a switch so that consumers can choose to always treat the Timezone as Invariant with the intent of reducing the application final size once it gets linked since all of the code in the framework that takes Timezones and compares them will be optimized out.

@eerhardt
Copy link
Member

@eerhardt at least the setting seems to already be present in the SDK so isn't this one done already?
What am I missing?

The InvariantGlobalization setting only affects the globalization data that we read from ICU. In relation to TimeZones, the only thing InvariantGlobalization affects is how we look up time zone names - we will use a non-localized name from the tzdata file. We will need a new setting here since we've shipped InvariantGlobalization already with the current TimeZone behavior.

This is something that is supported by Blazor 3.2 as an opt-in feature which we should not regress on.

In Blazor WASM 3.2, we shipped an MSBuild property named BlazorEnableTimeZoneSupport, which removes the dotnet.timezones.dat file from the app. That functionality is in 5.0-preview8 currently.

This work item is to introduce a new feature switch in the BCL which will disable Time Zone support altogether (even without the app being linked, as feature switches can be configured by AppContext). The feature switch will also be used to trim the TimeZoneInfo code in Blazor WASM, when enabled.

Since this is net-new functionality that changes behavior, and .NET 5 is closing down, I don't believe this is strictly necessary to ship .NET 5. The behavior from 3.2 is not regressed with the current bits.

In 6.0, we can introduce a new feature switch in this area.

cc @pranavkm

@eerhardt eerhardt modified the milestones: 5.0.0, 6.0.0 Jul 30, 2020
@marek-safar marek-safar added size-reduction Issues impacting final app size primary for size sensitive workloads and removed linkable-framework Issues associated with delivering a linker friendly framework labels Dec 9, 2020
@SamMonoRT
Copy link
Member

@eerhardt @pranavkm - Still seems to be in Uncommitted state - do we think this is possible to fit in for .NET6 ?

@eerhardt
Copy link
Member

eerhardt commented Jul 1, 2021

I don't believe this will fit into 6.0. Moving to Future.

@pavelsavara
Copy link
Member

After #82250 the BlazorEnableTimeZoneSupport feature is broken and this issue is good way how to make it work again.

@pavelsavara pavelsavara modified the milestones: Future, 8.0.0 May 23, 2023
@pavelsavara pavelsavara added the arch-wasm WebAssembly architecture label May 23, 2023
@ghost
Copy link

ghost commented May 23, 2023

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

For size sensitive apps it'd be very helpful to have an option (feature switch) to exclude full time-zones support. This is something that is supported by Blazor 3.2 as an opt-in feature which we should not regress on.

@eerhardt

Author: marek-safar
Assignees: pavelsavara
Labels:

arch-wasm, size-reduction, area-System.DateTime

Milestone: 8.0.0

@danmoseley
Copy link
Member

Is this potentially helpful for current AOT work @eerhardt .. or is it a subset of invariant globalization flag?

@eerhardt
Copy link
Member

is it a subset of invariant globalization flag?

See #37973 (comment)

Is this potentially helpful for current AOT work

It is minorly helpful. TimeZoneInfo has some code in it that can be trimmed, but it won't move the needle on a web app. TimeZoneInfo is already trimmed in a "Hello, World" app.

Using version 8.0.100-preview.5.23265.3 of the .NET SDK on win-x64 the rough cost of TimeZoneInfo is:

Console.WriteLine("Hello, World!"): 1.19 MB (1,257,984 bytes)
Console.WriteLine(DateTime.Now): 1.34 MB (1,405,952 bytes)

So TimeZoneInfo adds about 145 KB. For linux-x64 it is slightly larger at 180 KB.

The real savings here is when we can exclude the tzdata information. The only place I know of where this is helpful is browser-wasm apps. Everywhere else we get the tzdata information already on the OS.

@pavelsavara
Copy link
Member

On wasm tzdata is now bundled into .wasm file, about 220KB. Previously it was separate file which BlazorEnableTimeZoneSupport flag removed from download. In order to fix it, we need similar feature-switch in the runtime for wasm, which will skip bundling the TZ into the .wasm file (while recompiled with wasm-tools workload) and redirect BlazorEnableTimeZoneSupport into the new runtime flag.

@Clockwork-Muse
Copy link
Contributor

... given JS (and thus presumably wasm) has the Temporal proposal, which includes timezone info, would one route to take be to shim calls to the native library?

Obviously not out yet, also obviously you need an implementing browser. But it could mean you don't need to ship the timezones to clients (unless you really want to, to potentially ensure certain data, but that should normally be an edge case).

@danmoseley
Copy link
Member

So TimeZoneInfo adds about 145 KB. For linux-x64 it is slightly larger at 180 KB.

That's still ~2% of stage 1, right? Or it's already out of stage 1?

@eerhardt
Copy link
Member

TimeZoneInfo is in stage 1. That's why I said it is "minorly helpful", but it won't move the needle. Stage 1 is rather small compared to a real world web app. I don't believe we would turn this switch on by default.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 8, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 1, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.DateTime size-reduction Issues impacting final app size primary for size sensitive workloads
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants