Remove run-time calculation of hash constants in pdfium.

Remove run-time calculation of hash constants in pdfium.

PDFium static initializers must go. Static initializers are prohibited
by the style guide. They have negative consequences including increased
startup time (from pulling in additional code pages) and reduced sharing
of data pages (since the variables can't go in the read-only data
segment).

This change uses a template struct and typed enums to reproduce
JS_CalcHash at run-time. An unsigned long long constant and masking with
0xFFFFFFFF are used to avoid compile errors due to integer overflow of
compile-time constants.

The HashVerify class is used to check the results, necessary since none
of the functions in global.cpp are called when pdfium_test.exe runs.

const_expr would be a much cleaner way to implement this change but it
is not yet widely supported.

On the Windows release build this reduces the code size (.text
virtual size) by 0x240 (576) bytes, the .data section by 0x20 bytes
(for eight unsigned globals), and the .rdata section by 0x20 bytes
(the unneeded string savings, minus the eight unsigned globals now
being there).

BUG=441899
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/286ea1d88a41a670a17bd82d85a553c2df53c0c8

[brucedawson, 2014-12-30 20:44:43]

brucedawson@chromium.org changed reviewers:
+ tsepez@chromium.org

[Tom Sepez, 2015-01-02 17:59:08]

tsepez@chromium.org changed reviewers:
+ thestig@chromium.org

[Tom Sepez, 2015-01-02 17:59:08]

+Lei for some c++ expertise.

https://codereview.chromium.org/792043005/diff/1/fpdfsdk/src/javascript/globa...
File fpdfsdk/src/javascript/global.cpp (right):

https://codereview.chromium.org/792043005/diff/1/fpdfsdk/src/javascript/globa...
fpdfsdk/src/javascript/global.cpp:21: template<const wchar_t A = 0, const
wchar_t B = 0, const wchar_t C = 0,
Do we allow variadic templates from c++11x?  Does that help with your max arity
issue?

https://codereview.chromium.org/792043005/diff/1/fpdfsdk/src/javascript/globa...
fpdfsdk/src/javascript/global.cpp:24: struct cHash
Nit: struct/class name should start with uppercase.  Maybe a better name here?

https://codereview.chromium.org/792043005/diff/1/fpdfsdk/src/javascript/globa...
fpdfsdk/src/javascript/global.cpp:29: enum : unsigned { value = (X == 0)
can we do this with static const int value instead?  Is that cleaner?

[brucedawson, 2015-01-02 19:56:18]

I wrote an alternate implementation that uses variadic templates -- any thoughts
on which is better?

I also got rid of the enums and renamed the class.

There should also be some indication, perhaps a comment in fxjs_v8.h, about this
compile-time implementation of the hashing. The implementation could be put in
that header file, but I'm not convinced that that is worthwhile.

https://codereview.chromium.org/792043005/diff/1/fpdfsdk/src/javascript/globa...
File fpdfsdk/src/javascript/global.cpp (right):

https://codereview.chromium.org/792043005/diff/1/fpdfsdk/src/javascript/globa...
fpdfsdk/src/javascript/global.cpp:21: template<const wchar_t A = 0, const
wchar_t B = 0, const wchar_t C = 0,
We do allow variadic templates, they are used in base\tuple.h. They do solve the
max-arity issue, but it's not clear to me whether this is progress or not.

https://codereview.chromium.org/792043005/diff/1/fpdfsdk/src/javascript/globa...
fpdfsdk/src/javascript/global.cpp:24: struct cHash
Yep.

https://codereview.chromium.org/792043005/diff/1/fpdfsdk/src/javascript/globa...
fpdfsdk/src/javascript/global.cpp:29: enum : unsigned { value = (X == 0)
Yes, static const int is cleaner.

[brucedawson, 2015-01-02 21:04:25]

This change has been tested to ensure that it compiles with clang and that it
does not cause problems for /analyze (base\tuple.h causes internal compiler
errors in VC++ /analyze builds).

[Tom Sepez, 2015-01-05 20:14:09]

https://codereview.chromium.org/792043005/diff/20001/fpdfsdk/src/javascript/g...
File fpdfsdk/src/javascript/global.cpp (right):

https://codereview.chromium.org/792043005/diff/20001/fpdfsdk/src/javascript/g...
fpdfsdk/src/javascript/global.cpp:46: static const unsigned value = (N *
Pow<Ns...>::value + CHash<Ns...>::value)
Is there a way to avoid double recursion? We're going to be computing Pow<...>
over and over as we recurse into CHash<...>.  I see why you do this because of
ordering of paramenters vs. result.  What if we tried to add an 'accummulator'
parameter to the template to pass partial values down to the next template to
deal with this?

[brucedawson, 2015-01-05 21:19:23]

I totally agree that the double recursion is ugly. I'm getting the
hang of these templates now so I was able to implement your passed-in
accumulator idea. Updated.

Is CHash an adequate name, or would FX_CHash or some-such be better?

Also, should CHash be left in global.cpp (perhaps with a
comment in front of the implementation of FX_HashCode_String_GetW to
indicate that CHash exists) or should CHash be moved to fxjs_v8.h,
where JS_CalcHash is declared?

[Tom Sepez, 2015-01-05 21:22:09]

On 2015/01/05 21:19:23, brucedawson wrote:
> I totally agree that the double recursion is ugly. I'm getting the
> hang of these templates now so I was able to implement your passed-in
> accumulator idea. Updated.
> 
> Is CHash an adequate name, or would FX_CHash or some-such be better?
I think that's a fine name, given that its local to a .cpp file and
not included everywhere.

> 
> Also, should CHash be left in global.cpp (perhaps with a
> comment in front of the implementation of FX_HashCode_String_GetW to
> indicate that CHash exists) or should CHash be moved to fxjs_v8.h,
> where JS_CalcHash is declared?
Maybe later.  I'm OK with this for now.

[Tom Sepez, 2015-01-05 21:26:20]

Me too.  I was playing around with this because I was curious.

https://codereview.chromium.org/792043005/diff/40001/fpdfsdk/src/javascript/g...
File fpdfsdk/src/javascript/global.cpp (right):

https://codereview.chromium.org/792043005/diff/40001/fpdfsdk/src/javascript/g...
fpdfsdk/src/javascript/global.cpp:21: template <unsigned acc, wchar_t... Ns>
Should this be unsigned long long acc?
[nit: ACC in uppercase to match other params?]

https://codereview.chromium.org/792043005/diff/40001/fpdfsdk/src/javascript/g...
fpdfsdk/src/javascript/global.cpp:22: struct CHash {};
nit: do we need {} here?

https://codereview.chromium.org/792043005/diff/40001/fpdfsdk/src/javascript/g...
fpdfsdk/src/javascript/global.cpp:24: template <wchar_t N>
I think in this case, the argument is <unsinged long long ACC>

https://codereview.chromium.org/792043005/diff/40001/fpdfsdk/src/javascript/g...
fpdfsdk/src/javascript/global.cpp:39: CHash<0,'s','t','r','i','n','g'>::value;
Maybe use a helper class so that we don't have to explicitly pass 0.

[Tom Sepez, 2015-01-05 21:47:20]

Howzabout:


template <wchar_t... Ns> struct CHash;
template <unsigned long long ACC, wchar_t... Ns> struct CHashHelper;

template <unsigned long long ACC>
struct CHashHelper<ACC> {
  static const unsigned long long value = ACC;
};

template <unsigned long long ACC, wchar_t N, wchar_t... Ns>
struct CHashHelper<ACC, N, Ns...> {
  static const unsigned long long value = CHashHelper<ACC * 1313LLU + N,
Ns...>::value;
};

template <>
struct CHash<> {
  static const unsigned value = 0;
};

template <wchar_t N, wchar_t... Ns>
struct CHash<N, Ns...> {
  static const unsigned value = CHashHelper<0, N, Ns...>::value & 0xffffffff;
};

[brucedawson, 2015-01-05 21:52:56]

Now it is perfect.

https://codereview.chromium.org/792043005/diff/40001/fpdfsdk/src/javascript/g...
File fpdfsdk/src/javascript/global.cpp (right):

https://codereview.chromium.org/792043005/diff/40001/fpdfsdk/src/javascript/g...
fpdfsdk/src/javascript/global.cpp:21: template <unsigned acc, wchar_t... Ns>
ACC looks a bit ugly, but changed for consistency. I think unsigned is correct
(see below).

https://codereview.chromium.org/792043005/diff/40001/fpdfsdk/src/javascript/g...
fpdfsdk/src/javascript/global.cpp:22: struct CHash {};
I guess not. Removed.

https://codereview.chromium.org/792043005/diff/40001/fpdfsdk/src/javascript/g...
fpdfsdk/src/javascript/global.cpp:24: template <wchar_t N>
The goal is to always pass in unsigned -- making down to unsigned ASAP. I'm
pretty sure there is no need for unsigned long long.

https://codereview.chromium.org/792043005/diff/40001/fpdfsdk/src/javascript/g...
fpdfsdk/src/javascript/global.cpp:39: CHash<0,'s','t','r','i','n','g'>::value;
No need for a helper class -- I just deleted the zeroes. They were added during
an investigation but are totally unneeded. Good catch.

[brucedawson, 2015-01-05 21:58:15]

It's an addictive intellectual puzzle isn't it?

I'm inclined to prefer my solution -- four instances of one class is
simpler than five instances of two classes.

I'm tempted to simplify mine by dropping support for hashing one-character
strings, but I think I will leave it for completeness.

[Tom Sepez, 2015-01-05 22:00:27]

lgtm

[brucedawson, 2015-01-05 22:11:04]

Committed patchset #4 (id:60001) manually as
286ea1d88a41a670a17bd82d85a553c2df53c0c8 (presubmit successful).