Closed Bug 180851 Opened 22 years ago Closed 21 years ago

remove unnecessary converters for Non-Unix or Xft builds

Categories

(Core :: Internationalization, defect)

Product:
Core
Component:
Internationalization
Platform:
All
Windows 2000
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jshin1987, Assigned: jshin1987)

References

Details

(Keywords: intl, memory-footprint)

Attachments

(2 files, 6 obsolete files)

a patch per Seawood's comment
29.64 KB, patch
netscape
: review+
alecf
: superreview+
Details | Diff | Splinter Review
a patch for configure.in : moving test after XPRINT/XCOREFONT test
1.90 KB, patch
netscape
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; ko-KR; rv:1.1) Gecko/20020826
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; ko-KR; rv:1.1) Gecko/20020826

Some converters in uconv module are only necessary for Unix/Linux X11
(they're for X11 core font encodings), but
they're still compiled in for Windows and Mac OS. Removing them
would save us a bit of memory footprint. They are

  nsUnicodeToX11Johab
  nsUnicodeToGBKNoAscii
  nsUnicodeToJohabNoAscii
  nsUnicodeToKSC5601    
  nsUnicodeToJISx0208
  nsUnicodeToJISx0212



Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Actual Results:  
With converters listed above removed, the memory footprint
would shrink
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: intl
Attached patch a patch (obsolete) — — Splinter Review 
The reduction in memory footprint is very modest as expected.
It's only about 8kB. For the optimized build on MS WIndows, uconv.dll is 
765952 bytes after removing converters for X11 core fonts.
Before that, it was 774144 bytes. I haven't tried it
under Linux, yet. When Xft is enabled, those converters for
X11 core fonts are not necessary as is the case of MacOS
and Windows. 
Incidentally, I found that MOZ_ENABLE_COREXFONTS is turned on
even for Windows and presumably for MacOS. 
It's harmless, but if we think it's
worth while to save this modest amount, we have to
use 'NO_X11' instead. To disable those converters
for Xft(Linux) build, perhaps we have to use
'NO_X11 || MOZ_ENABLE_XFT' (because NO_X11 is off
on Unix/Linux).  

FYI, converters turned off are 

  UnicodeToJISX0208
  UnicodeToJISX0212
  UnicodeToJohabNoAscii
  UnicodeToX11Johab
  UnicodeToKSC5601
  UnicodeToBig5NoASCII
  UnicodeToGBKNoASCII

Pls, take a look and let me know if this small amount is worth 
reducing.  (for embedding, it may not be so small as it sounds...)
alecf, do embedding people care about this patch?
Keywords: footprint
Comment on attachment 107218 [details] [diff] [review]
a patch 

This is great!
but is NO_X11 really what we're looking for? I don't think NO_X11 is defined on
windows/mac...

How about
#if defined(XP_UNIX) && !defined(NO_X11)

?
configure.in:    HOST_CFLAGS="$HOST_CFLAGS -DXP_BEOS -DBeOS -DBEOS
-D_POSIX_SOURCE -DNO_X11"
configure.in:    HOST_CFLAGS="$HOST_CFLAGS -TC -nologo -DXP_PC -DXP_WIN32
-DXP_WIN -DWIN32 -D_WIN32 -DNO_X11"
configure.in:    HOST_CFLAGS="$HOST_CFLAGS -DXP_UNIX -DXP_MACOSX -DNO_X11"
configure.in:    HOST_CFLAGS="$HOST_CFLAGS -DXP_OS2 -DNO_X11"

So BeOS, Windows, Mac OS X and OS/2 all define it.

The only one i'm curious about is Photon. I'd have to resurrect my QNX box to
find out.
As you all may have noticed, my patch(attachment 107218 [details] [diff] [review]) has

+#ifdef NO_X11
 #include "nsUnicodeToGBKNoAscii.h"
+#endif

What I really meant was '#ifndef NO_X11'(or
'#if !defined(NO_X11)'). Apparently when I edited
the patch (replacing 'MOZ_ENABLE_COREXFONTS' with 'NO_X11'),
I forgot to reverse the logic. 

As I wrote in comment #1, we also need to disable it for Mozilla-Xft.
Given what alecf and timeless wrote, we can use
either of the following (the latter is as good as the former
provided that NO_X11 is defined on all non-Unix platforms and
MacOSX)

if defined(XP_UNIX) && !defined(NO_X11) && !defined(MOZ_ENABLE_XFT)

if !defined(NO_X11) && !defined(MOZ_ENABLE_XFT)

 

I'm fine with NO_X11 given timeless's findings. I would like to see an updated
patch with the logic correct though...just so we're all on the same page :)
Now I'm using |if !defined(NO_X11) && !defined(MOZ_ENABLE_XFT)|
Attachment #107218 - Attachment is obsolete: true
In case of Mozilla-Xft build(on my Linux box), the reduction in the size of
libuconv.so
is larger than that for Mozilla-Win.
Before the removal, it was 4,958,708 bytes. It's 4,825,676. Hmm...
this is strange, though. How come the shared obj. is so much bigger
on Linux opt. build than dll for Windows? Aha... that's because
I turned on debug-flag for my optimized build....
Summary: remove unnecessary converters for Windows and MacOS → remove unnecessary converters for Non-Unix or Xft builds
sorry for spamming. I forgot to change Makefile.in in attachment 115508 [details] [diff] [review]. 
The size comparison is still valid because cpp files excluded were not
linked in anyway.
Attachment #115508 - Attachment is obsolete: true
Attachment #115514 - Flags: superreview?(alecf)
Attachment #115514 - Flags: review?(timeless)
Comment on attachment 115514 [details] [diff] [review]
a new patch(with Makefile.in fixed as well)

ok, this looks fine

stylistically i'd prefer for you to have $(NULL) at the end of each list
 +CPPSRCS		+= \
 +		nsUnicodeToKSC5601.cpp \
 +		nsUnicodeToX11Johab.cpp \
-+		nsUnicodeToJohabNoAscii.cpp 
++		nsUnicodeToJohabNoAscii.cpp \
++		$(NULL)
 +endif

The reason we use $(NULL) is to preserve cvs blame, it's just empty whitespace
(not a terminator). It means that someone could add a single line to the end of
your minilist without getting blame for the last item you had in it.

of course this is mostly a non issue since it's very unlikely that we'd ever
add anything to these lists. so this comment is mostly informational/for future
reference. feel free to take it or leave it.
Attachment #115514 - Flags: review?(timeless) → review+
Comment on attachment 115514 [details] [diff] [review]
a new patch(with Makefile.in fixed as well)

What timeless said.

sr=alecf
I can't wait for this to land! :)
Comment on attachment 115514 [details] [diff] [review]
a new patch(with Makefile.in fixed as well)

Erm...
-- snip --
+
+ifndef NO_X11
+ifndef MOZ_ENABLE_XFT
+CPPSRCS		+= \
-- snip --
This can't be correct for two reasons:
1. What happens if someone uses X core fonts and XFT in one build and chooses
the font system via prefs ?
2. We still need such encoders for Xprint until Xft for Mozilla's Xprint module
has been implemented.
Attachment #115514 - Flags: review+ → review-
3rd reason (IMHO not the last one):
-- snip --
ifndef NO_X11
-- snip --
will fail for ports like Qt (what about Photon ?!) which may be used without X11
but may require these converters.

... and what about the case when we build multiple toolkits (remember the bug
that we want to have a cmd-line option to select the toolkits (e.g.
GTK+/GTK+v2/GTK+Xft/Xlib/Qt/etc.)) ?
All right. Let's forget about MOZ_ENABLE_XFT for the moment.
How about this?

if defined(XP_UNIX) && !defined(XP_MACOSX)

and an equivalent in Makefile.in
This would minimize # of platform/toolkit combinations
without converters in question while still removing them
on major platforms(MS Win, OS/2, MacOS(X), BeOS)
where they're not necessary.

A riskier alternative is to tighten up the usage pattern
of MOZ_ENABLE_COREXFONTS, define it only where it's used
and use |ifdef MOZ_ENABLE_COREXFONTS|. (Xprint, Qt
and so forth should define it if they need to use it)
Currently, it's even turned on for MS Windows build.
 
alecf, timeless, and Roland
why don't you pick your favorite? I'm inclined toward
the former (a safer one)



it's probably too much work, but i'd vote for MOZ_ENABLE_COREFONTS :)
i don't care but it'd certainly be cleaner in all of the places where you do checks.
Assignee: smontagu → jshin
Jungshik Shin wrote:
> All right. Let's forget about MOZ_ENABLE_XFT for the moment.
> How about this?
>
> if defined(XP_UNIX) && !defined(XP_MACOSX)
>
> and an equivalent in Makefile.in
> This would minimize # of platform/toolkit combinations
> without converters in question while still removing them
> on major platforms(MS Win, OS/2, MacOS(X), BeOS)
> where they're not necessary.

What about splitting this bug into two steps - first using |defined(XP_UNIX) &&
!defined(XP_MACOSX)| and then narrowing down the finer details later... :)

> A riskier alternative is to tighten up the usage pattern
> of MOZ_ENABLE_COREXFONTS, define it only where it's used
> and use |ifdef MOZ_ENABLE_COREXFONTS|. (Xprint, Qt
> and so forth should define it if they need to use it)
> Currently, it's even turned on for MS Windows build.

|MOZ_ENABLE_COREXFONTS| is currently pretty much useless for non-gfx/src/gtk/ 
build stuff - only the GTK+ gfx recognises this flag yet - and using it
elsewhere (like in intl/) will cause trouble for those port (owners) who did not
know that symbol is getting a meaning outside gfx/src/gtk/, too... ;-/

What about using something like
-- snip --
if ((defined(MOZ_ENABLE_GTK) || defined(MOZ_ENABLE_GTK2)) 
    && defined(MOZ_ENABLE_COREXFONTS)) ||
    defined(MOZ_ENABLE_XLIB) || 
    defined(MOZ_ENABLE_QT) || 
    defined(MOZ_ENABLE_XPRINT)
-- snip --
? :)
actually yeah.. why don't we just consolidate all the logic somewhere global
like config.mk or autoconf.mk - defining a new #ifdef for it. instead of
sprinkling this long winded XP_UNIX && !NO_X11 || XP_MACOSX logic (and yes I'm
joking, I realize its not as complicated as I wrote above...)
Given what Roland wrote and the way it's set in configure.in,
it doesn't look so desirable to overload MOZ_ENABLE_COREXFONTS 
to have an additional meaning outside gfx/src/gtk|gtk2. 

I guess we have two options:

- use |if defined(XP_UNIX) && !defined(XP_MACOSX)|
  and forget about Xft.


- add the following to configure.in and 
  use |ifdef  MOZ_ENABLE_EXTRAX11CONVERTERS|. 

if (test "$MOZ_ENABLE_GTK"  || test "$MOZ_ENABLE_GTK2") \
&& test "$MOZ_ENABLE_COREXFONTS" \
|| test "$MOZ_ENABLE_XLIB" \
|| test "$MOZ_ENABLE_QT" \
|| test "$MOZ_ENABLE_XPRINT"
then
    AC_DEFINE(MOZ_ENABLE_EXTRAX11CONVERTERS)
fi




Qt is no longer in the build, so with the exception of the QT definition above,
I'd say we go with the 2nd option.
Alec Flett wrote:
> Qt is no longer in the build, so with the exception of the QT definition >
above, I'd say we go with the 2nd option.

People are still working on Qt in a branch, so _please_ do not shut the doors
for them until they either migrate their work back to "trunk" or give up.
Jungshik Shin wrote:
> Given what Roland wrote and the way it's set in configure.in,
> it doesn't look so desirable to overload MOZ_ENABLE_COREXFONTS 
> to have an additional meaning outside gfx/src/gtk|gtk2. 
>
> I guess we have two options:
> - use |if defined(XP_UNIX) && !defined(XP_MACOSX)|
>  and forget about Xft.

That's the failsafe option, however I guess alecf will bite me when I'll vote
for that... :)

> - add the following to configure.in and 
>  use |ifdef  MOZ_ENABLE_EXTRAX11CONVERTERS|. 
>
> if (test "$MOZ_ENABLE_GTK"  || test "$MOZ_ENABLE_GTK2") \
> && test "$MOZ_ENABLE_COREXFONTS" \
> || test "$MOZ_ENABLE_XLIB" \
> || test "$MOZ_ENABLE_QT" \
> || test "$MOZ_ENABLE_XPRINT"
> then
>    AC_DEFINE(MOZ_ENABLE_EXTRAX11CONVERTERS)
> fi

Looks good... :)
1. |MOZ_ENABLE_QT| is currently not being used in "trunk" (it's currently only
available in the Qt branch), but please either comment the |test
"$MOZ_ENABLE_QT"| out (/* later */) or add a comment in the patch that this
needs to be enabled for Qt, too.
2. Please add a comment in the patch for "configure.in" which code is affected
by |MOZ_ENABLE_EXTRAX11CONVERTERS| (e.g. "turns off converters in intl/ which
are used only by native X11 gfx implementations" (or something like that :))
my autotools(autoconf/automake) are not compatible with Mozilla's
configure.in. 

Alec, can you regenerate 'configure' with autoconf 2.12? Otherwise,
I'll install old autotools and try to regenerate 'configure'.

Per Roland's command, I didn't remove QT.
Attachment #115514 - Attachment is obsolete: true
I juste manually added a few lines to 'configure' and it looks like
it's working fine.
Attachment #115685 - Attachment is obsolete: true
Comment on attachment 115685 [details] [diff] [review]
a new patch (with configure.in change)

s/MOZ_ENABLE_EXTRAX11CONVERTERS/MOZ_USE_EXTRAX11CONVERTERS/ - I am not sure
which one is better... or |MOZ_DISABLE_EXTRAX11CONVERTERS| since the _default_
is to enable them.
> the _default_ is to enable them.

 It's to be turned on only
for those platform/toolkit combinations that need them.
Because it's not an independent configure option
(i.e. we're not interested in allowing 
'./configure --enable-extrax11converters', are we?)
but dependent on other MOZ_ENABLE_*'s, I'm inclined
to
   s/MOZ_ENABLE_EXTRAX11CONVERTERS/MOZ_EXTRA_X11CONVERTERS/

, which I think is more in line with the way other
_dependent_ configure 'variables' are dealt with.

Either way is fine with me, though. Let's wrap up this simple
fix.
Attachment #115686 - Attachment is obsolete: true
I didn't use MOZ_USE_... because it's  used only once by
MOZ_USE_RTTI and ...
cc'ing seawood - I thought QT was gone.. but maybe there is a better #define for
what we're looking for anyway...
Comment on attachment 115693 [details] [diff] [review]
added comment, QT removed(put in comment), etc

Please don't include configure changes in future patches.  (It bloats the patch
and they rarely apply cleanly.) 

And remove the line about Qt from configure.in. Qt is gone for the moment.  If
it returns,then they can add a test just like they would for the other places
that we currently only test for gtk, gtk2 & xlib.  Btw, the current assumption
is that enabling qt, gtk, gtk2 or xlib means that you're building on X11. 
There are non-X11 ports of these toolkits but we don't support them.  We never
have and it'll take more than just changing this configure test to add that
support.  Any alternate toolkit that isn't X11 based should be setting NO_X11 &
-DNO_X11 or that's a bug in the toolkit implementation.  FYI, we're not going
to support building with multiple toolkits RSN (like as soon as I get a r= for
the patch in bug 191528) so that's no longer a concern either.

The patch only sets the -DMOZ_EXTRA_X11CONVERTERS cpp define.  You still need
to add the AC_SUBST to configure.in & the line to autoconf.mk so that the rest
of the Makefiles see the env define.
Attachment #115693 - Flags: review-
can you take a look?
Attachment #115693 - Attachment is obsolete: true
Comment on attachment 115729 [details] [diff] [review]
a patch per Seawood's comment

Change:

+then
+    AC_DEFINE(MOZ_EXTRA_X11CONVERTERS)
+fi

to

+then
+    AC_DEFINE(MOZ_EXTRA_X11CONVERTERS)
+    MOZ_EXTRA_X11CONVERTERS=1
+fi

&& r=cls
Comment on attachment 115729 [details] [diff] [review]
a patch per Seawood's comment

Thank you for help and r, seawood
I'll add the line mentioned by seawood.
BTW, what's the procedure to check in
'configure'? Am I trusted to generate
'configure' without a mistake out
of 'configure.in' and check it in?
Attachment #115729 - Flags: superreview?(alecf)
There's a server side cronjob which automatically regenerates the toplevel
configure whenever the toplevel configure.in is checked in.  If you have access
to a stock autoconf 2.13, then you can check in your generated configure script,
otherwise just let the cronjob handle it.


Thanks for the answer. Once I get sr,
I'll just commit configure.in and
leave to crond  to gen. configure because
my stock autconf is 2.5x. 
Comment on attachment 115729 [details] [diff] [review]
a patch per Seawood's comment

yay! Glad to see this done right.
sr=alecf

requesting a review from seawood just so we're all on the same page.
Attachment #115729 - Flags: superreview?(alecf)
Attachment #115729 - Flags: superreview+
Attachment #115729 - Flags: review?(seawood)
Comment on attachment 115729 [details] [diff] [review]
a patch per Seawood's comment

Add the line I mentioned before && r=cls
Attachment #115729 - Flags: review?(seawood) → review+
fix checked in. thank you all.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
I'm sorry I didn't realize XPRINT and XCOREFONT test come AFTER
my test in configure.in.
Obviously, it should be put after XPRINT and XCOREFONT.
Javier H Pedemonte at IMB kindly informed me of this blunder.
seawood and alecf, can you give this a quick r/sr stamp?
reopening until a new configure.in is checked in.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 115940 [details] [diff] [review]
a patch for configure.in : moving test after XPRINT/XCOREFONT test

r=cls
Attachment #115940 - Flags: review+
Attachment #115514 - Flags: superreview?(alecf)
fix checked-in. I should've known that seawood's r is sufficient for configure.in
Status: REOPENED → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
was this done correctly?  "Linux comet Dep" shows a significant increase in
codesize, all attributable to libuconv:

  Zdiff:+5652 (+5948/-296)
 mZdiff:+5652 (+5948/-296)

i'm not sure if comet is building Xft or not, but i would have expected a
decrease in codesize instead of an increase.  what's up with that? ;-)
Yes, there should have been an increase.  The last checkin fixed the problem of
not enabling the extra converters if xprint was enabled.  Xprint is enabled by
default if you're building for an X11 toolkit & you have -lXp.  When the
original checkin was made, there was probably a matching signficant decrease on
comet.


Christopher Seawood wrote:
> Yes, there should have been an increase.  The last checkin fixed the problem 
> of not enabling the extra converters if xprint was enabled. 

Er, this is IMHO not Xprint-specific.
The missing converters rendered our japanese font support useless on the main
display, too (see bug 195792 ("Japanese text is displayed in Chinese font")). As
long we want to support server-side X11 fonts we need those converters (e.g. for
the next 5-10 years :) ...
Blocks: 195792
Why are we building xprint by default?  Isn't it pretty much just extra bloat?  
I don't know a single person who uses XPrint (aside from Roland).  If we're 
adding extra bloat just for something that no one uses anyways, why don't we 
disable xprint and save some space?
Like Roland said, this so-called "bloat" is not just caused by xprint.  Look at
the test.  Building with gtk{,2} & corexfonts will also cause the converters to
be built.  corexfonts are always on unless you specifically disable them by
setting MOZ_ENABLE_COREXFONTS=0 .

sounds reasonable to me.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: