1225401 - Remove cities timezone data from ICU

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect)

Description

[Makoto Kato [:m_kato]]

My understand is that DateTimeFormat doesn't allow city data.  But our ICU dat still has this data.

If removing this, it reduces 400KB and Chrome already removes it.

Comment 1

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=055fc164c85e

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Remove cities from time zone data

I should wait the review until landing ICU 56.1 update.

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Part 1. Update update-icu.sh to remove cities from zone data

Comment 4

[Makoto Kato [:m_kato]]

Attachment: Part 2. Remove cities from ICU data

Comment 5

[Makoto Kato [:m_kato]]

Comment on attachment 8691695 [details] [diff] [review]
Part 1. Update update-icu.sh to remove cities from zone data

Our Intl API implementation doesn't allow that time zone is city name.  So city names are unnecessary.

Also Chromium already removes cities from ICU data to reduce data size.

Comment 6

[Makoto Kato [:m_kato]]

Comment on attachment 8691696 [details] [diff] [review]
Part 2. Remove cities from ICU data

This is diff by remove script.   I can reduce ~400KB binary size.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8691695 [details] [diff] [review]
Part 1. Update update-icu.sh to remove cities from zone data

Review of attachment 8691695 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/update-icu.sh
@@ +48,5 @@
> +# Remove cities in timezone data.
> +for i in ${icu_dir}/source/data/zone/*.txt
> +do
> +  grep -E '^        "meta:' $i > /dev/null
> +  if [ $? -eq 0 ]; then

IRC kibitzes that this would be better written as:

if grep -q -E '^        "meta:' $i; then

Please do so.  :-)

@@ +49,5 @@
> +for i in ${icu_dir}/source/data/zone/*.txt
> +do
> +  grep -E '^        "meta:' $i > /dev/null
> +  if [ $? -eq 0 ]; then
> +    sed -i '/^    zoneStrings/, /^        "meta:/ {

IRC alleges: "I believe that's: between lines matching zoneStrings and meta: if the line matches zoneStrings, print; if the line matches meta, print; otherwise delete".  That would correspond with the patch changes, at least based one examining one or two files.  Is that description correct?  If so add a comment explaining this.  I propose the following:

"""
Between lines matching the given zoneStrings and meta patterns, inclusive: if the line matches the given zoneStrings pattern, include (print) it; if the line matches the given meta pattern, include (print) it; otherwise remove (delete) it.
"""

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 8691696 [details] [diff] [review]
Part 2. Remove cities from ICU data

Review of attachment 8691696 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me assuming the previous patch's comments from me understood the patch and its effects correctly.

Comment 9

[Jeff Walden [:Waldo]]

Also, it'd be nice to sync this up with what v8 does, so the two can be compared by the interested reader.  Did the sed command do something like what v8 does?  Could you include a link to what v8 does in the patch, so the interested reader can easily compare the approaches?

Comment 10

[Steven R. Loomis]

OK, so please file something upstream in ICU. You might add comments to http://bugs.icu-project.org/trac/ticket/10922  - we need to get requirements like this. I said the same to Chrome people.

The internals of ICU's data should NOT be considered stable.

Comment 11

[(not currently active) Ted Mielczarek]

FYI, these patches are going to be obsoleted by bug 1239083.

Comment 12

[Makoto Kato [:m_kato]]

Currently our implementation of ECMA-402 supports Etc/UTC only.

But, ECMA-402 2.0 (http://www.ecma-international.org/ecma-402/2.0/#sec-time-zone-names) allows IANA Time Zone Database too.  So for 2.0, I might not have to remove these localization data...  So I keep open state.

Comment 13

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2e23c79cae660ad22ecab81ec67d5cdff0262b4

Comment 14

[Makoto Kato [:m_kato]]

Attachment: Part 1. Remove cities when syncing ICU source code

Comment 15

[Makoto Kato [:m_kato]]

Attachment: Part 2. Update ICU source in tree after removing cities data

Comment 16

[Makoto Kato [:m_kato]]

Attachment: Part 3. Update icudt58l.dat

Comment 17

[Makoto Kato [:m_kato]]

Comment on attachment 8840355 [details] [diff] [review]
Part 1. Remove cities when syncing ICU source code

Update script for ICU 58.2.  Cities data seems to be is still unused.

Comment 18

[Makoto Kato [:m_kato]]

Comment on attachment 8840357 [details] [diff] [review]
Part 2. Update ICU source in tree after removing cities data

Update ICU code into our tree after running Part 1's script.

Comment 19

[Makoto Kato [:m_kato]]

Comment on attachment 8840358 [details] [diff] [review]
Part 3. Update icudt58l.dat

Update ICU data into our tree after running Part 1's script.

Comment 20

[Jeff Walden [:Waldo]]

Can this stuff still be removed, now that we support the timeZone property on DateTimeFormat options?  It looks like all time zone information is recorded in intl/icu/source/data/misc/zoneinfo64.txt now, and so touching these files shouldn't affect our semantics.  Am I correct about that?  Would like to know that before I try too hard to review any of this.

Comment 21

[Makoto Kato [:m_kato]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #20)
> Can this stuff still be removed, now that we support the timeZone property
> on DateTimeFormat options?  It looks like all time zone information is
> recorded in intl/icu/source/data/misc/zoneinfo64.txt now, and so touching
> these files shouldn't affect our semantics.  Am I correct about that?  Would
> like to know that before I try too hard to review any of this.

Yes, it works timeZone property like new Date().toLocaleTimeString("en", {timeZone: "America/New_York"}) even if removing localized city name.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/292d9e369264
Part 1. Remove cities when syncing ICU source code. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/6696016a0198
Part 2. Update ICU source in tree after removing cities data. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c51fd25a389
Part 3. Update icudt58l.dat. r=Waldo

Comment 23

[Makoto Kato [:m_kato]]

ah, I will back out due to confict updating with Bug 1343493.  After recreate dat file, land again.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1c9c38cdeac8ab930c62b8c7492fa557afd4a691
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aa81a5b064ea43d2ec0379734788369e403ea3b8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/94985ca794f129d05880f8cb035430664411dd24

Comment 24

[Zibi Braniecki [:zbraniecki][:gandalf]]

do we know how much this shaves off of the apk size hit of ICU?

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45e867a1c89e
Part 1. Remove cities when syncing ICU source code. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/c056f2674719
Part 2. Update ICU source in tree after removing cities data. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc40078f0ac6
Part 3. Update icudt58l.dat. r=Waldo

Comment 26

[Makoto Kato [:m_kato]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #24)
> do we know how much this shaves off of the apk size hit of ICU?

around 140KB.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/45e867a1c89e
https://hg.mozilla.org/mozilla-central/rev/c056f2674719
https://hg.mozilla.org/mozilla-central/rev/cc40078f0ac6

Comment 28

[William Lachance (:wlach)]

Some apk size wins from this:

== Change summary for alert #5297 (as of March 03 2017 04:57 UTC) ==

Improvements:

  0%  installer size summary android-4-0-armv7-api15 opt        34966457.08 -> 34834684.25
  0%  installer size summary android-api-15-gradle opt          35323165.33 -> 35193592.33
  0%  installer size summary android-4-0-armv7-api15 debug      36108531.42 -> 35978278.67
  0%  installer size summary android-4-2-x86 opt                37471828.38 -> 37337097.58

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=5297