Closed Bug 709657 Opened 8 years ago Closed 8 years ago

Slice out character converters from libxul

Categories

(Core :: Internationalization, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX
mozilla11

People

(Reporter: khuey, Assigned: khuey)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
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.
You will need to not split it out on android, as the dynamic linker can't load components from the apk yet.
Ugh, really?  That's going to make doing this for any XPCOM component a total PITA.
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.
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)
Attached patch PatchSplinter Review
The leak was caused by the deadlock detector :-/

I also (think I) made this Windows only.
Attachment #580828 - Attachment is obsolete: true
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
Attachment #580979 - Flags: review?(mh+mozilla) → review+
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?
In intl/uconv/src/Makefile.in, line 50, it looks like you missed a paren in "ifeq (WINNT,$(OS_TARGET)".
Yep, there's a few other problems too.  I've using try to sort those out.
https://hg.mozilla.org/mozilla-central/rev/271d2711b66c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
++khuey
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?
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.
(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.
Blocks: 750661
You need to log in before you can comment on or make changes to this bug.