Enable RTTI for ICU....

Enable RTTI for ICU.

ICU 4.6 uses RTTI (dynamic_cast and typeid) in many places. 

It'll increase the vtbl size. I'm gonna measure the size impact by
making two builds:

1. With rtti enabled
2. With rtti disabled, but with dynamic_cast replaced with
static_cast and typeid with 'something' buildable. This build wouldn't run
properly, but for the size comparision, just compiling it should be fine.

On Windows, we set _HAS_EXCEPTIONS to 0 in common.gypi. To build ICU
without enabling exception as well on Windows, we replaced the following line 

#include <typeinfo>

with 

#include "unicode/utypeinfo.h"

and add a new file 'common/unicode/utypeinfo.h' with the following:


#if defined(_MSC_VERSION) && _HAS_EXCEPTIONS == 0
#include <exception>
using std::exception;
#endif 
#include <typeinfo>


BUG=61514
TEST=On all 3 platforms, Chrome can be built and can be run without crash.



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75186

[jungshik at Google, 2011-02-10 19:43:17]

This is for ICU 4.6, which is not yet pulled in. 

I have yet to measure the size impact on all 3 platforms.

[M-A Ruel, 2011-02-10 19:55:08]

You can just compare the size with the current version of ICU in optimized
build.

Also make sure you are building this library with WPO.

It is probably that it is not necessary to enable exception. You should try
since the saving will be significant.

[Mark Mentovai, 2011-02-10 19:59:29]

LGTM for the experiments, but not LGTM for an actual live release until your
investigation is complete.

[jungshik at Google, 2011-02-12 03:02:45]

On 2011/02/10 19:59:29, Mark Mentovai wrote:
> LGTM for the experiments, but not LGTM for an actual live release until your
> investigation is complete.

We have 3 options:

1. (patch set 2) : Enable RTTI and exception handling for ICU and set
_HAS_EXCEPTIONS to 1 for ICU
2. (patch set 3) : Enable RTTI and set _HAS_EXCEPTIONS to 1 for ICU, but not
enable exception
3. (patch set 4 : that I'm gonna upload): Enable RTTI for ICU, but leave alone
_HAS_EXCEPTIONS. Instead, whenever <typeinfo> is included, put the following
right before it:

#if defined(_HAS_EXCEPTIONS) && !_HAS_EXCEPTIONS
#include <exception>
using std::exception;
#endif

[jungshik at Google, 2011-02-15 01:28:06]

Patch set 5 does the following:
- enable RTTI
- disable exception handling
- To get ICU built on WIndows with _HAS_EXCEPTIONS=0, whenever typeinfo is
included, exception is included and 'using std::exception' is added

It gives the best result in terms of the size among RTTI-enabled builds. 

I measured the size of chrome.dll, Chromium Framekwork and 'chrome' on Win, Mac
and Linux, respectively. All of them are chromium release builds. 

1. ICU 4.6 : RTTI vs no RTTI 

RTTI increases the size by 37k, 24k and 45k on Win, Mac and Linux. Note that no
RTTI builds crash whenever RTTI is actually used (e.g. in History page). no RTTI
build is made with http://codereview.chromium.org/6520018/

2. ICU 4.6 RTTI vs ICU 4.2.1

ICU 4.6 RTTI build is 39k larger than ICU 4.2.1 on Win. However, icudt46.dll is
1.97M smaller than icudt42.dll

ICU 4.6 RTTI build is 1.86M smaller on Mac than ICU 4.2.1.
ICU 4.6 RTTI build is 1.84M smaller on Linux than ICU 4.2.1.

On Mac and Linux, icudata is statically linked and offsets (and swamps) the
modest increase due to RTTI (a few tens of kB).  On Windows, icudata is in a
separate DLL. 

ICU data size got much smaller in ICU 4.6 because I took advantage of the new
data organization in ICU 4.6 to shave off unused data. 

On Linux and Mac, the sizes of recent official dev channel builds are comparable
to those of my release builds with ICU 4.2.1 or ICU 4.6. So, I guess using my
release builds as a size benchmark should suffice. 

Mark and Evan, do you agree to the above? 

On Windows, the size difference between my release build and a recent dev
channel official build (installed on my machine) is significant (the latter is
about 10MB / 30% smaller). So, I think I have to build official builds with
different configurations to see the size impact in WPO-enabled builds. 

On the other hand, ICU data size reduction is likely to swamp the size increase
due to RTTI even in WPO-enabled build. 

maruel, do you still think it's necessary to build official builds on Windows?

[jungshik at Google, 2011-02-15 01:33:35]

The raw data for the measurements is at 

https://spreadsheets.google.com/pub?key=0AqR-4TI2U6sidHZPNFdpQXR2WmRFMDl5THNM...

[Mark Mentovai, 2011-02-15 02:17:59]

Patch set 5 is LGTM in as far as the Mac is concerned, and the overall principle
is LGTM. My questions:

1. Do you want to update a README to reflect the code changes you’ve made, or
provide a .patch file?

1a. Are you planning on upstreaming the code changes?

2. I don’t know much about _HAS_EXCEPTIONS. I see “using std::exception” used as
a boilerplate, but I looked at a few files and couldn’t find any use of
“exception” anywhere. Can you explain how this is supposed to operate and why
it’s effective? I assume that it’s simply that MSVC++’s <typeinfo> relies on
“exception” being available as an identifier.

[jungshik at Google, 2011-02-15 07:50:47]

Thank you for the review. 

On 2011/02/15 02:17:59, Mark Mentovai wrote:
> Patch set 5 is LGTM in as far as the Mac is concerned, and the overall
principle
> is LGTM. My questions:
> 
> 1. Do you want to update a README to reflect the code changes you’ve made, or
> provide a .patch file?

Yes, I do. I've been punting updating README in ICU 4.6. I'll update it asap I'm
done with this. 


> 1a. Are you planning on upstreaming the code changes?
> 

I've  filed an upstream bug earlier today (I meant to add a reference to it, but
forgot). It's http://bugs.icu-project.org/trac/ticket/8343 I'm not sure what the
upstream thinks of it, though. 




> 2. I don’t know much about _HAS_EXCEPTIONS. I see “using std::exception” used
as
> a boilerplate, but I looked at a few files and couldn’t find any use of
> “exception” anywhere. Can you explain how this is supposed to operate and
why
> it’s effective? I assume that it’s simply that MSVC++’s <typeinfo> relies on
> “exception” being available as an identifier.

Yes, this is purely to make MSVC++ happy when exception handling is disabled
(i.e. /EscH+ is not specified) and _HAS_EXCEPTIONS is set to 0, which is what
common.gypi does in Chrome. We do not want to enable exception handling even
ONLY for  ICU (that increased the binary size further). As you found out, ICU
does NOT use exception.

An alternative is patch set 3 (set _HAS_EXCEPTIONS to 1 in icu.gyp). The
chrome.dll size does not change, but I got a warning about 'exception unwinding
not working'. Perhaps, that's benign (because ICU doesn't use C++ exception).

[Marc-Antoine Ruel (Google), 2011-02-15 13:48:43]

I think it'd be nicer to have a typeinfo.h that would do the wrap up but I don't
really mind at that point, having it upstreamed would be the most important
thing to reduce pain.

So the change itself LGTM. Sorry for the pain.

http://codereview.chromium.org/6484008/diff/14001/third_party/icu/source/comm...
File third_party/icu/source/common/rbbi.cpp (right):

http://codereview.chromium.org/6484008/diff/14001/third_party/icu/source/comm...
third_party/icu/source/common/rbbi.cpp:13: #if defined(_MSC_VER) &&
defined(_HAS_EXCEPTIONS) && !_HAS_EXCEPTIONS
I think  
#if defined(_MSC_VER) && _HAS_EXCEPTIONS == 0
would work and would be a tad shorter?

[jungshik at Google, 2011-02-16 21:30:41]

On 2011/02/15 13:48:43, Marc-Antoine Ruel (Google) wrote:
> I think it'd be nicer to have a typeinfo.h that would do the wrap up but I
don't
> really mind at that point, having it upstreamed would be the most important
> thing to reduce pain.
> 
> So the change itself LGTM. Sorry for the pain.
> 
>
http://codereview.chromium.org/6484008/diff/14001/third_party/icu/source/comm...
> File third_party/icu/source/common/rbbi.cpp (right):
> 
>
http://codereview.chromium.org/6484008/diff/14001/third_party/icu/source/comm...
> third_party/icu/source/common/rbbi.cpp:13: #if defined(_MSC_VER) &&
> defined(_HAS_EXCEPTIONS) && !_HAS_EXCEPTIONS
> I think  
> #if defined(_MSC_VER) && _HAS_EXCEPTIONS == 0
> would work and would be a tad shorter?

Thank you for the suggestion. In patch set 6, I did that (and added a new file
utypeinfo.h and refer to it in individual files). I've also suggested that to
the upstream, too. 

As for the size impact,  to make sure, I build official builds on Windows and as
expected, the size increase is modest (34.8kB) and is swamped by the ICU data
size reduction. 

I'll land patch set 6. Thank you, all.