Skip to content

Tz size fix#116

Merged
tqiu8 merged 7 commits intodotnet:masterfrom
tqiu8:tz-size-fix
Feb 12, 2021
Merged

Tz size fix#116
tqiu8 merged 7 commits intodotnet:masterfrom
tqiu8:tz-size-fix

Conversation

@tqiu8
Copy link
Contributor

@tqiu8 tqiu8 commented Feb 11, 2021

Reduce size of timezone data, by compiling zic from IANA and using -b slim for tz compilation. Fixes: dotnet/runtime#48061

@CoffeeFlux
Copy link

What are we seeing size-wise with this change?

<RemoveDir Directories="$(TimeZoneDataSrcDir)" />
<DownloadFile SourceUrl="https://data.iana.org/time-zones/tzdb-$(TimeZoneDataVersion)/%(Area.Identity)"
DestinationFolder="$(TimeZoneDataIntermediateDir)" />
<Exec Command="wget -r -nH --cut-dirs=2 --no-parent --reject='index.html*' -P $(TimeZoneDataIntermediateDir) https://data.iana.org/time-zones/tzdb-2021a/" />
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<Exec Command="wget -r -nH --cut-dirs=2 --no-parent --reject='index.html*' -P $(TimeZoneDataIntermediateDir) https://data.iana.org/time-zones/tzdb-2021a/" />
<Exec Command="wget -r -nH --cut-dirs=2 --no-parent --reject='index.html*' -P $(TimeZoneDataIntermediateDir) https://data.iana.org/time-zones/tzdb-$(TimeZoneDataVersion)/" />

<DownloadFile SourceUrl="https://data.iana.org/time-zones/tzdb-$(TimeZoneDataVersion)/%(Area.Identity)"
DestinationFolder="$(TimeZoneDataIntermediateDir)" />
<Exec Command="wget -r -nH --cut-dirs=2 --no-parent --reject='index.html*' -P $(TimeZoneDataIntermediateDir) https://data.iana.org/time-zones/tzdb-2021a/" />
<Exec Command="touch README" WorkingDirectory="$(TimeZoneDataIntermediateDir)"/>
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to download the entire tzdb directory instead of just specific files. make zic doesn't work without a README, which is not (weirdly) included in the directory itself.

Copy link

@CoffeeFlux CoffeeFlux Feb 12, 2021

Choose a reason for hiding this comment

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

...huh, is there any chance of PRing a fix for that upstream? This is fine as a workaround, but we should look into why that is.

Copy link
Member

@lewing lewing Feb 12, 2021

Choose a reason for hiding this comment

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

It's possible we should switch to using the full tar.lz that contains everything no.

@lewing
Copy link
Member

lewing commented Feb 12, 2021

What are we seeing size-wise with this change?

Early results are good, we're a little worried that the runtime tests aren't exhaustive enough.

@eerhardt
Copy link
Member

we're a little worried that the runtime tests aren't exhaustive enough.

Can you elaborate on this? Specifically what concerns do you have? In my experience, the TimeZoneInfo tests are pretty decent.

@lewing
Copy link
Member

lewing commented Feb 12, 2021

we're a little worried that the runtime tests aren't exhaustive enough.

Can you elaborate on this? Specifically what concerns do you have? In my experience, the TimeZoneInfo tests are pretty decent.

We have absolutely run into several problems by targeting the tests too narrowly (for size savings) and we're just trying to avoid that happening again.

lewing
lewing previously requested changes Feb 12, 2021
Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

This changes the build targets that aren't actually being run as part of CI, but it doesn't update the data that is generated and consumed on the dotnet/runtime side

@lewing lewing self-requested a review February 12, 2021 17:05
@lewing lewing dismissed their stale review February 12, 2021 17:06

issue addressed

@ViktorHofer
Copy link
Member

So the binary diff is somewhere between 20-70% based on what GitHub displays. That's great to see.

Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

Looks good now. @ViktorHofer the final compressed size comes in at about 43796 bytes which is solid savings.

@tqiu8 tqiu8 merged commit 4e4ab3f into dotnet:master Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dotnet.timezones.blat file size has increased 34% since net5.0

5 participants