273424 - Option to disable obsolete vtable support [minimo]

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Darin Fisher]

Option to disable obsolete vtable support

Minimo and other products that don't care about binary compatibility with
Mozilla 1.4 can benefit from eliminating nsTAString::mVTable and all of the
extra code associated with it.

The solution is simple: implement all methods in nsTAString.h as inline wrappers
around the corresponding methods on nsTSubstring or nsTString.

This should give a significant codesize and performance benefit.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

>The solution is simple: implement all methods in nsTAString.h as inline wrappers
>around the corresponding methods on nsTSubstring or nsTString.

hm, why not just:
  typedef nsSubstring nsString;
?

Comment 2

[Darin Fisher]

I suppose you meant: typedef nsSubstring nsAString, right?  The problem is that
people have coded |class nsAString;| all over the place.  I tried to get rid of
that before, by replacing them with |#include "nsStringFwd.h"|, but it became
very difficult.  (I don't remember the details.)  Maybe it could still be done.
 The maintenance problem is compounded since this would only be an optional
build mode.  Of course, we do have a minimo tinderbox! ;-)

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

er, oops -  yes, that is what I meant.

Comment 4

[Brian Ryner (not reading)]

Attachment: one way to do this

This is one way we could do this with minimal code changes.  I keep the classes
pretty much the same, but I use the preprocessor to rename nsTAString_CharT to
nsSubstring_base, and nsTSubstring_CharT to nsAString.	I then #ifdef out all
of the members and methods on nsSubstring_base, leaving just the typedefs. 
This was to avoid duplicating the typedefs.  Most of the other changes were
just removing duplicate methods (anything using abstract_string_type where
there was also a substring_type version).

Code size savings is about 60KB on windows and about 170KB on Mac (trunk
Firefox).  Haven't had a chance to test Linux, I'd estimate it's somewhere in
the middle of those.

Comment 5

[Darin Fisher]

Comment on attachment 170897 [details] [diff] [review]
one way to do this

>Index: configure.in

>+MOZ_ARG_DISABLE_BOOL(v1-string-abi,
>+[  --disable-v1-string-abi   Disable binary compatibility layer for strings],
>+    MOZ_1X_STRING_ABI=,

nit: some inconsistency here... "v1" vs "1X"... can they be the same?


>Index: xpcom/string/public/nsTSubstringTuple.h

>+#else
>+      typedef nsTSubstring_CharT         abstract_string_type;
>+#endif
...
>+operator+(const nsTSubstringTuple_CharT::abstract_string_type& a, const nsTSubstringTuple_CharT::abstract_string_type& b)

nit: would it be better to use operator_type here as well?


Nice work!  r+sr=darin

Comment 6

[Darin Fisher]

When this lands, I'll throw this switch in the minimo-arm build on neptune. 
That'll help ensure that it doesn't bit-rot, and we'll get to see how much it
helps codesize with that build.

Comment 7

[Brian Ryner (not reading)]

Attachment: revised patch

Changed MOZ_1X_STRING_ABI to MOZ_V1_STRING_ABI, and changed use of
nsTSubstringTuple_CharT::abstract_string_type to just string_type.

Comment 8

[Brian Ryner (not reading)]

Attachment: use base_string_type instead of operator_string_type

After discussion with darin, decided to use base_string_type instead of
string_type in nsTSubstringTuple and instead of operator_type in nsTSubstring.

Comment 9

[Brian Ryner (not reading)]

checked in

Comment 10

[Hiro]

The following errors occur when the "ac_add_options --disable-v1-string-abi"
option is applied.
https://bugzilla.mozilla.org/show_bug.cgi?id=235499#c39

Comment 11

[Darin Fisher]

I enabled this option on the neptune minimo-arm tinderbox today, and it appears
to have saved about 110k on the size of the resulting binary.