Move all static constants in dtoa/cached-powers.* to .rodata section

Move all static constants in dtoa/cached-powers.* to .rodata section

Move all static constants in dtoa/cached-powers.* from .data section to
.rodata. This was done by marking those as const and hardcoding their
values instead of computing them. Runtime assertions were added to make
sure the hardcoded values are correct and kept up-to-date.

This data can now be shared between different library instances on
Android, thus saving RAM.

The size of non-shareable .data in wtf/dtoa/wtf.cached-powers.o went from 1424
bytes to 0 byte. The size of shareable .rodata went from 760 bytes to 2520
bytes.

BUG=249746
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161250

[Inactive, 2013-10-28 14:11:08]

Proof via objdump:
Before:
022e0850 l     O .data	00000570
WTF::double_conversion::initialize()::cachedPowers

After:
0000000005707be0 l     O .rodata	0000000000000570             
WTF::double_conversion::initialize()::cachedPowers

The size of non-shareable .data in wtf/dtoa/wtf.cached-powers.o went from 1424
bytes to 20 bytes. The size of shareable .rodata went from 760 bytes to 2168
bytes.

[digit1, 2013-10-28 14:21:02]

ltgm

However, this specific source file is depressing, there is absolutely no good
reason to use mutable variables here.

It would be much better to make everything constant, and have a compile or
runtime assert that checks that kDecimalExponentDistance, kMinDecimalExponent
and kMaxDecimalExponent have the appropriate values with regards to the constant
kCachedPowers table.

The constant values are even suspicious to me. I'd feel better if they would be
generated from a Python script, or something easier to understand (even if this
is done one-time, not on every build).
And there is no unit test that check they are correct :-(

[tfarina, 2013-10-28 14:31:46]

On Mon, Oct 28, 2013 at 12:11 PM,  <ch.dumez@samsung.com> wrote:
> Reviewers: eseidel, digit1,
>
> Message:
> Proof via objdump:
Would you mind including the full command line you used? So others
less versed in objdump could benefit and learn from it?

> Before:
> 022e0850 l     O .data  00000570
> WTF::double_conversion::initialize()::cachedPowers
>
> After:
> 0000000005707be0 l     O .rodata        0000000000000570
> WTF::double_conversion::initialize()::cachedPowers
>
> The size of non-shareable .data in wtf/dtoa/wtf.cached-powers.o went from
> 1424
> bytes to 20 bytes. The size of shareable .rodata went from 760 bytes to 2168
> bytes.
>
I'd put all these as well in the CL description.

> Description:
> Move WTF's cachedPowers array from .data to .rodata section
>
> Move WTF's cachedPowers array from .data to .rodata section by making it
> const.
> As a result, this data can be shared between different instances of the
> library
> on Android, thus saving RAM.
>
> BUG=249746
>
> Please review this at https://codereview.chromium.org/48833002/
>
> SVN Base: https://chromium.googlesource.com/chromium/blink.git@master
>
> Affected files (+2, -2 lines):
>   M Source/wtf/dtoa/cached-powers.cc
>
>
> Index: Source/wtf/dtoa/cached-powers.cc
> diff --git a/Source/wtf/dtoa/cached-powers.cc
> b/Source/wtf/dtoa/cached-powers.cc
> index
>
22fda4b913c83d0913e600dc570d05bcd67552a2..f628347b12a845fac21bd21023bab0e05ad12bc4
> 100644
> --- a/Source/wtf/dtoa/cached-powers.cc
> +++ b/Source/wtf/dtoa/cached-powers.cc
> @@ -48,7 +48,7 @@ namespace double_conversion {
>      static int kCachedPowersLength = 1;
>      static int kCachedPowersOffset = 1;
>      static const double kD_1_LOG2_10 = 0.30102999566398114;  //  1 / lg(10)
> -    static CachedPower* kCachedPowers = 0;
> +    static const CachedPower* kCachedPowers = 0;
>
>      int PowersOfTenCache::kDecimalExponentDistance = 1;
>      int PowersOfTenCache::kMinDecimalExponent = 1;
> @@ -57,7 +57,7 @@ namespace double_conversion {
>      void initialize() {
>          if (kCachedPowers)
>              return;
> -        static CachedPower cachedPowers[] = {
> +        static const CachedPower cachedPowers[] = {
>              {UINT64_2PART_C(0xfa8fd5a0, 081c0288), -1220, -348},
>              {UINT64_2PART_C(0xbaaee17f, a23ebf76), -1193, -340},
>              {UINT64_2PART_C(0x8b16fb20, 3055ac76), -1166, -332},
>
>



-- 
Thiago

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Inactive, 2013-10-28 14:37:11]

I updated the description.

On 2013/10/28 14:31:46, tfarina wrote:
> On Mon, Oct 28, 2013 at 12:11 PM,  <mailto:ch.dumez@samsung.com> wrote:
> > Reviewers: eseidel, digit1,
> >
> > Message:
> > Proof via objdump:
> Would you mind including the full command line you used? So others
> less versed in objdump could benefit and learn from it?

Here is the command I used to validate:
$ objdump -x out/Release/lib/libchromeview_prebuilt.so | grep cachedPowers |
c++filt
022e0850 l     O .data	00000570
WTF::double_conversion::initialize()::cachedPowers

To get everything in .data or .data.rel* for WebCore:
objdump -x out/Release/lib/libchromeview_prebuilt.so | grep "\.data" | grep
WebCore | c++filt

[Inactive, 2013-10-28 14:39:59]

On 2013/10/28 14:37:11, Chris Dumez wrote:
> I updated the description.
> 
> On 2013/10/28 14:31:46, tfarina wrote:
> > On Mon, Oct 28, 2013 at 12:11 PM,  <mailto:ch.dumez@samsung.com> wrote:
> > > Reviewers: eseidel, digit1,
> > >
> > > Message:
> > > Proof via objdump:
> > Would you mind including the full command line you used? So others
> > less versed in objdump could benefit and learn from it?
> 
> Here is the command I used to validate:
> $ objdump -x out/Release/lib/libchromeview_prebuilt.so | grep cachedPowers |
> c++filt
> 022e0850 l     O .data	00000570
> WTF::double_conversion::initialize()::cachedPowers
> 
> To get everything in .data or .data.rel* for WebCore:
> objdump -x out/Release/lib/libchromeview_prebuilt.so | grep "\.data" | grep
> WebCore | c++filt

The following script is also very useful to get the number of relocations at
load time:
http://www.akkadia.org/drepper/relinfo.pl

Relocations usually slow down library loading so we should try to avoid them
when possible.

[abarth-chromium, 2013-10-29 03:45:37]

It's easy to generate CPP from Python these days.

[Inactive, 2013-10-29 16:00:07]

I moved all the global variables to .rodata as suggested. I am not generating
anything with Python yet because I am not 100% sure it is worth the effort (how
often will we update these values?)

Please take another look.

[digit1, 2013-10-29 16:21:47]

lgtm, thanks for this excellent patch. Don't worry about the Python for now :)

[Inactive, 2013-10-30 13:16:32]

On 2013/10/29 16:21:47, digit1 wrote:
> lgtm, thanks for this excellent patch. Don't worry about the Python for now :)

Could a wtf/ OWNER please take a look as well? Thanks!

[Inactive, 2013-11-01 21:03:00]

On 2013/10/30 13:16:32, Chris Dumez wrote:
> On 2013/10/29 16:21:47, digit1 wrote:
> > lgtm, thanks for this excellent patch. Don't worry about the Python for now
:)
> 
> Could a wtf/ OWNER please take a look as well? Thanks!

+cevans for wtf/ in case Eric is busy or out of office.

[eseidel, 2013-11-01 21:16:56]

lgtm

Wow.

[eseidel, 2013-11-01 21:17:50]

https://codereview.chromium.org/48833002/diff/160001/Source/wtf/dtoa/cached-p...
File Source/wtf/dtoa/cached-powers.cc (right):

https://codereview.chromium.org/48833002/diff/160001/Source/wtf/dtoa/cached-p...
Source/wtf/dtoa/cached-powers.cc:50: {UINT64_2PART_C(0xfa8fd5a0, 081c0288),
-1220, -348},
I agree with Adam that these should probably be generated at some point.  That
doesn't have to be this patch.

[commit-bot: I haz the power, 2013-11-01 21:21:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/48833002/160001

[commit-bot: I haz the power, 2013-11-01 22:06:58]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_blink_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-04 13:52:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/48833002/340001

[commit-bot: I haz the power, 2013-11-04 15:51:54]

Change committed as 161250

[Nico, 2013-11-04 20:55:03]

https://codereview.chromium.org/48833002/diff/340001/Source/wtf/dtoa/cached-p...
File Source/wtf/dtoa/cached-powers.cc (right):

https://codereview.chromium.org/48833002/diff/340001/Source/wtf/dtoa/cached-p...
Source/wtf/dtoa/cached-powers.cc:138: static const int kCachedPowersLength =
ARRAY_SIZE(kCachedPowers);
This isn't used for anything. Is that intentional?

[Inactive, 2013-11-04 20:59:27]

https://codereview.chromium.org/48833002/diff/340001/Source/wtf/dtoa/cached-p...
File Source/wtf/dtoa/cached-powers.cc (right):

https://codereview.chromium.org/48833002/diff/340001/Source/wtf/dtoa/cached-p...
Source/wtf/dtoa/cached-powers.cc:138: static const int kCachedPowersLength =
ARRAY_SIZE(kCachedPowers);
On 2013/11/04 20:55:03, Nico wrote:
> This isn't used for anything. Is that intentional?

This is used in 2 assertions in this file so I believe we need to keep it. We
could - however - put it in an #ifndef NDEBUG.

[Nico, 2013-11-04 21:04:59]

(for BUG=312287 )

https://codereview.chromium.org/48833002/diff/340001/Source/wtf/dtoa/cached-p...
File Source/wtf/dtoa/cached-powers.cc (right):

https://codereview.chromium.org/48833002/diff/340001/Source/wtf/dtoa/cached-p...
Source/wtf/dtoa/cached-powers.cc:138: static const int kCachedPowersLength =
ARRAY_SIZE(kCachedPowers);
On 2013/11/04 20:59:28, Chris Dumez wrote:
> On 2013/11/04 20:55:03, Nico wrote:
> > This isn't used for anything. Is that intentional?
> 
> This is used in 2 assertions in this file so I believe we need to keep it. We
> could - however - put it in an #ifndef NDEBUG.

Ah. I think we usually use `#if !ASSERT_DISABLED` in blink. Can you add that?
Else this file won't compile with warnings enabled.

[Inactive, 2013-11-04 21:12:40]

On 2013/11/04 21:04:59, Nico wrote:
> (for BUG=312287 )
> 
>
https://codereview.chromium.org/48833002/diff/340001/Source/wtf/dtoa/cached-p...
> File Source/wtf/dtoa/cached-powers.cc (right):
> 
>
https://codereview.chromium.org/48833002/diff/340001/Source/wtf/dtoa/cached-p...
> Source/wtf/dtoa/cached-powers.cc:138: static const int kCachedPowersLength =
> ARRAY_SIZE(kCachedPowers);
> On 2013/11/04 20:59:28, Chris Dumez wrote:
> > On 2013/11/04 20:55:03, Nico wrote:
> > > This isn't used for anything. Is that intentional?
> > 
> > This is used in 2 assertions in this file so I believe we need to keep it.
We
> > could - however - put it in an #ifndef NDEBUG.
> 
> Ah. I think we usually use `#if !ASSERT_DISABLED` in blink. Can you add that?
> Else this file won't compile with warnings enabled.

Sure, I am preparing a CL right now. Thanks!