709657 - Slice out character converters from libxul

Status: RESOLVED WONTFIX

(Core :: Internationalization, defect)

Description

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

Attachment: Patch

We were looking for some code to toss out of libxul to relieve the pressure (Bug 709193) and gal suggested uconv.  Some of the uconv stuff uses internal string APIs and other fun stuff, but the character converters can be split out pretty easily.  The attached patch compiles and passes the tests in intl/uconv on my machine.  I threw it at try overnight to see what happens.

Comment 1

[Mike Hommey [:glandium]]

You will need to not split it out on android, as the dynamic linker can't load components from the apk yet.

Comment 2

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

Ugh, really?  That's going to make doing this for any XPCOM component a total PITA.

Comment 3

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

This patch makes libxul 300KB smaller than it is on mozilla-beta (total savings of 600ish KB).  It unfortunately has a small leak, and we need to solve the Android issues.

Comment 4

[Mike Hommey [:glandium]]

Bug 683127 will fix the android issue ; maybe not on the short term, though.

Note that we don't need to make libxul smaller on other platforms, this could well be windows-only (which is what i'm doing for bug 709721, for instance)

Comment 5

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

Attachment: Patch

The leak was caused by the deadlock detector :-/

I also (think I) made this Windows only.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 580979 [details] [diff] [review]
Patch

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

rs=me. At this time of day, I don't trust myself for an r+ on such a patch.

+EXTRA_DSO_LDOPTS += \
+  $(XPCOM_GLUE_LDOPTS) \
+  $(XPCOM_LIBS) \
+  $(MOZ_JS_LIBS) \

You probably don't need MOZ_JS_LIBS

::: js/xpconnect/shell/xpcshell.cpp
@@ +1746,5 @@
>  
>  int
>  main(int argc, char **argv, char **envp)
>  {
> +    Sleep(30000);

You don't want that :)

::: testing/xpcshell/runxpcshelltests.py
@@ +311,5 @@
>        Simple wrapper to launch a process.
>        On a remote system, this is more complex and we need to overload this function.
>      """
>      cmd = wrapCommand(cmd)
> +    print cmd

That neither

::: toolkit/xre/nsAppRunner.cpp
@@ +2580,5 @@
>  
>  int
>  XRE_main(int argc, char* argv[], const nsXREAppData* aAppData)
>  {
> +  Sleep(10000);

That neither

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 580979 [details] [diff] [review]
Patch

>--- a/intl/uconv/src/Makefile.in
>+++ b/intl/uconv/datamodule/Makefile.in
> SHARED_LIBRARY_LIBS += \
> 	../ucvlatin/$(LIB_PREFIX)ucvlatin_s.$(LIB_SUFFIX) \
> 	../ucvibm/$(LIB_PREFIX)ucvibm_s.$(LIB_SUFFIX) \
> 	../ucvja/$(LIB_PREFIX)ucvja_s.$(LIB_SUFFIX) \
> 	../ucvtw2/$(LIB_PREFIX)ucvtw2_s.$(LIB_SUFFIX) \
> 	../ucvtw/$(LIB_PREFIX)ucvtw_s.$(LIB_SUFFIX) \
> 	../ucvko/$(LIB_PREFIX)ucvko_s.$(LIB_SUFFIX) \
> 	../ucvcn/$(LIB_PREFIX)ucvcn_s.$(LIB_SUFFIX) \
>+  ../util/external/$(LIB_PREFIX)ucvxutil_s.$(LIB_SUFFIX) \

Get rid of the tabs on the other lines?

Comment 8

[Kelsey Gilbert [:jgilbert]]

In intl/uconv/src/Makefile.in, line 50, it looks like you missed a paren in "ifeq (WINNT,$(OS_TARGET)".

Comment 9

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

Yep, there's a few other problems too.  I've using try to sort those out.

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/271d2711b66c

Comment 11

[Andreas Gal :gal]

++khuey

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

khuey: How does this patch not regress startup time? Glancing at the patch, it seems like *all* the character converters get pulled out. Is it really the case that no character conversions (even ASCII<->UTF8,UTF16<->UTF8) are done during startup?

Comment 13

[Mike Hommey [:glandium]]

The addition of *one* binary component is not going to affect startup time dramatically. If it helps linkage, it's a good tradeoff. Turns out it doesn't help linkage, so it could be backed out.

Comment 14

[Philip Chee]

comm-central patch by khuey:
http://hg.mozilla.org/comm-central/rev/9652bb00432d

Comment 15

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

(In reply to Brian Smith (:bsmith) from comment #12)
> khuey: How does this patch not regress startup time? Glancing at the patch,
> it seems like *all* the character converters get pulled out. Is it really
> the case that no character conversions (even ASCII<->UTF8,UTF16<->UTF8) are
> done during startup?

This keeps UTF8<->UTF16 in libxul (though it does toss out the ASCII one apparently).

Anyways, it doesn't help, so it's coming out.

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/9df183d280ba
https://hg.mozilla.org/mozilla-central/rev/245c74c2f4dd

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to Philip Chee from comment #14)
> comm-central patch by khuey:
> http://hg.mozilla.org/comm-central/rev/9652bb00432d

Undone too in:
http://hg.mozilla.org/comm-central/rev/f72675b4a48f