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

Fix TimeZoneInfo Perf #85615

Merged
merged 2 commits into from
May 4, 2023
Merged

Fix TimeZoneInfo Perf #85615

merged 2 commits into from
May 4, 2023

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented May 1, 2023

Fixes #85432

This change is addressing multiple performance issues:

The first problem arises when creating a TimeZoneInfo object with an alternative zone ID (like using IANA Id on Windows and Windows Id on Linux) using the FindSystemTimeZoneById method. By default, the object returned by GetSystemTimeZones is cached only for the zone ID that matches the OS we are running on. Thus, when we try to create a new object with an alternative ID, we cannot benefit from the cache, and we always attempt to read the zone from the system. On non-Windows OS, this requires accessing the file system and probing for the zone file. To address this issue, we introduce a cache for the alternative ID.

The second issue concerns the unnecessary creation of new zone objects when requesting them with FindSystemTimeZoneById. Even if we have already created the object and cached it, we still create a new one every time. However, since the zone objects do not have writable properties, there is no need to clone them. Instead, we can return the cached object to avoid the allocation and the validation of the new object.

Windows

Before

|         Method |         Mean |      Error |     StdDev |   Gen0 | Allocated |
|--------------- |-------------:|-----------:|-----------:|-------:|----------:|
|    UsingIanaId | 40,061.32 ns | 753.377 ns | 667.849 ns |      - |     312 B |
| UsingWindowsId |     50.31 ns |   1.018 ns |   1.461 ns | 0.0095 |      80 B |

After

|         Method |     Mean |    Error |   StdDev | Allocated |
|--------------- |---------:|---------:|---------:|----------:|
|    UsingIanaId | 34.41 ns | 0.377 ns | 0.315 ns |         - |
| UsingWindowsId | 33.55 ns | 0.462 ns | 0.410 ns |         - |

Linux

Before

|         Method |       Mean |    Error |   StdDev |   Gen0 | Allocated |
|--------------- |-----------:|---------:|---------:|-------:|----------:|
|    UsingIanaId | 1,429.8 ns | 27.28 ns | 29.19 ns | 0.0095 |      80 B |
| UsingWindowsId |   935.2 ns | 18.36 ns | 21.86 ns | 0.0687 |     576 B |

After

|         Method |     Mean |    Error |   StdDev | Allocated |
|--------------- |---------:|---------:|---------:|----------:|
|    UsingIanaId | 31.87 ns | 0.634 ns | 1.143 ns |         - |
| UsingWindowsId | 41.03 ns | 0.838 ns | 1.229 ns |         - |

@ghost
Copy link

ghost commented May 1, 2023

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

Issue Details

Fixes #85432

This issue is addressing multiple performance issues:

The first problem arises when creating a TimeZoneInfo object with an alternative zone ID (like using IANA Id on Windows and Windows Id on Linux) using the FindSystemTimeZoneById method. By default, the object returned by GetSystemTimeZones is cached only for the zone ID that matches the OS we are running on. Thus, when we try to create a new object with an alternative ID, we cannot benefit from the cache, and we always attempt to read the zone from the system. On non-Windows OS, this requires accessing the file system and probing for the zone file. To address this issue, we introduce a cache for the alternative ID.

The second issue concerns the unnecessary creation of new zone objects when requesting them with FindSystemTimeZoneById. Even if we have already created the object and cached it, we still create a new one every time. However, since the zone objects do not have writable properties, there is no need to clone them. Instead, we can return the cached object to avoid the allocation and the validation of the new object.

Windows

Before

|         Method |         Mean |      Error |     StdDev |   Gen0 | Allocated |
|--------------- |-------------:|-----------:|-----------:|-------:|----------:|
|    UsingIanaId | 40,061.32 ns | 753.377 ns | 667.849 ns |      - |     312 B |
| UsingWindowsId |     50.31 ns |   1.018 ns |   1.461 ns | 0.0095 |      80 B |

After

|         Method |     Mean |    Error |   StdDev | Allocated |
|--------------- |---------:|---------:|---------:|----------:|
|    UsingIanaId | 34.41 ns | 0.377 ns | 0.315 ns |         - |
| UsingWindowsId | 33.55 ns | 0.462 ns | 0.410 ns |         - |

Linux

Before

|         Method |       Mean |    Error |   StdDev |   Gen0 | Allocated |
|--------------- |-----------:|---------:|---------:|-------:|----------:|
|    UsingIanaId | 1,429.8 ns | 27.28 ns | 29.19 ns | 0.0095 |      80 B |
| UsingWindowsId |   935.2 ns | 18.36 ns | 21.86 ns | 0.0687 |     576 B |

After

|         Method |     Mean |    Error |   StdDev | Allocated |
|--------------- |---------:|---------:|---------:|----------:|
|    UsingIanaId | 31.87 ns | 0.634 ns | 1.143 ns |         - |
| UsingWindowsId | 41.03 ns | 0.838 ns | 1.229 ns |         - |
Author: tarekgh
Assignees: -
Labels:

area-System.DateTime

Milestone: -

@tarekgh tarekgh added this to the 8.0.0 milestone May 1, 2023
@tarekgh
Copy link
Member Author

tarekgh commented May 1, 2023

@stephentoub could you please help review this one? Thanks!

@tarekgh tarekgh merged commit fbdf54c into dotnet:main May 4, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TimeZoneInfo.FindSystemTimeZoneById is slow on Linux when input is Windows TimeZone ID
2 participants