944348 - Algorithmic ICU encoding converters are being built even though we don't use them

Status: NEW

(Firefox Build System :: General, defect)

Description

[Henri Sivonen (:hsivonen)]

When building on Linux 64 with clang, I noticed something about "bocu" scroll by in the terminal. Looking  at the mach  printout, it turns out that we are building parts of ICU that we don't use, including encoding converters.

In the interest of build time and binary size, we should stop building parts of ICU that we don't use. Also, we absolutely should not give rise to circumstances that could actually expose cruft like BOCU-1 to the Web.

Comment 1

[Gregory Szorc [:gps]]

Ehsan: Since you are neck deep in ICU foo, is this something you could tackle? Is there a better bugzilla component for ICU?

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'd like us to end up moving all the ICU stuff to Core::Internationalization.

Comment 3

[(no longer active)]

(In reply to Gregory Szorc [:gps] from comment #1)
> Ehsan: Since you are neck deep in ICU foo, is this something you could
> tackle? Is there a better bugzilla component for ICU?

If somebody does all of the investigation necessary to figure out what kinds of flags we need to be passing to ICU and which parts of it will indeed be useless after Gecko starts to use it, then I don't mind writing the patch, but I'm not very familiar with the build system and I've spent _way_ more time in bug 915735 than any regular build system hacker would have so far, so I think a build system hacker really needs to own this.

Comment 4

[Masatoshi Kimura [:emk]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #2)
> I'd like us to end up moving all the ICU stuff to Core::Internationalization.

Yes, bug 724540. But we can disable the unused stuff until we actually start to nee it.

Comment 5

[Nicholas Nethercote [inactive]]

Not on B2G, and we don't have measurements on how much it'll save. MemShrink:P3 for now.

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: WIP

Comment 7

[Henri Sivonen (:hsivonen)]

So ICU tools don't build with this patch. It looks like we need the tools to package the ICU data appropriately.

It may be that the minimize what we ship, we first need to compile one copy of ICU for the build system itself and then another smaller copy that we ship.

I hope not, but that's what this looks like.

Comment 8

[Henri Sivonen (:hsivonen)]

I tried this further. And, yes, building data that we care about depends at least on the executable genrb, which itself depends on the ICU encoding converters existing.

Comment 9

[Henri Sivonen (:hsivonen)]

We should probably file an upstream bug about making the tools for generating the non-converter data to build with UCONFIG_NO_CONVERSION=1.

Comment 10

[Henri Sivonen (:hsivonen)]

Fixing this in our copy of ICU would involve way too many changes. For sanity, it makes more sense to get UCONFIG_NO_CONVERSION=1 fixed upstream.

Upstream bug report:
http://bugs.icu-project.org/trac/ticket/10868

Comment 11

[Henri Sivonen (:hsivonen)]

Comment on attachment 8412418 [details] [diff] [review]
WIP

Once we have an upstream fix, we should change UCONFIG_NO_LEGACY_CONVERSION to UCONFIG_NO_CONVERSION in icu.m4.

Comment 12

[Henri Sivonen (:hsivonen)]

Fortunately, we are currently building only algorithmic converters that don't need data tables.

Comment 13

[Simon Montagu :smontagu]

So http://bugs.icu-project.org/trac/ticket/10868 got closed as "working as designed" and apparently comment 7 is correct: to fix his we need to build ICU twice!

If the size gains are significant this would still be a net win even at the expense of increasing build time IMHO, especially if it helps unblock bug 864843.

Comment 14

[Mike Hommey [:glandium]]

FWIW, we already build twice for cross-compiles, I guess the setup would be similar.

Comment 15

[(not currently active) Ted Mielczarek]

FYI bug 1239083 will change things here, since we won't be building the ICU host tools during the Mozilla build. We'll have a pre-built copy of the ICU data file in-tree (built using the normal ICU configure && make), and we'll just build ICU for the target system using moz.build files.

Comment 16

[(not currently active) Ted Mielczarek]

That is, the patch from comment 11, applied to the not-yet-landed config/external/icu/defs.mozbuild, ought to work fine now.