[objects] seed NumberDictionary (only ia32 now)

[objects] seed NumberDictionary (only ia32 now)


BUG=
TEST=
R=erik.corry@gmail.com

[indutny, 2012-01-09 13:54:54]

sorry, patch is incomplete/incorrect now, fixing it right now

[indutny, 2012-01-09 16:05:12]

I think it works now, can you please review?

[indutny, 2012-01-09 16:17:51]

I can add const bool variable UsesSeed to all shapes.
And check it before calling GetHeap()->StringHashSeed() , that'll definitely be
optimized by most of C++ compilers (unused arguments are optimized only by g++,
as bnoordhuis told me).

What do you think?

[Erik Corry, 2012-01-09 23:52:47]

http://codereview.chromium.org/9148006/diff/1011/src/ia32/macro-assembler-ia3...
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/9148006/diff/1011/src/ia32/macro-assembler-ia3...
src/ia32/macro-assembler-ia32.cc:997: lea(r1, Operand(seed, RelocInfo::NONE));
No reason to use lea here when mov would do.

http://codereview.chromium.org/9148006/diff/1011/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/9148006/diff/1011/src/objects-inl.h#newcode2060
src/objects-inl.h:2060: uint32_t entry = FirstProbe(Shape::Hash(key,
GetHeap()->StringHashSeed()),
isolate->heap() is faster than GetHeap()
If the arguments don't all fit on one line they should have a line each.

http://codereview.chromium.org/9148006/diff/1011/src/objects-inl.h#newcode4544
src/objects-inl.h:4544: uint32_t NumberDictionaryShape::HashForObject(uint32_t
key, uint32_t seed,
Formatting.

http://codereview.chromium.org/9148006/diff/1011/src/objects-inl.h#newcode4569
src/objects-inl.h:4569: uint32_t StringDictionaryShape::HashForObject(String*
key, uint32_t,
Formatting.

http://codereview.chromium.org/9148006/diff/1011/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/9148006/diff/1011/src/objects.h#newcode2760
src/objects.h:2760: static inline uint32_t HashForObject(HashTableKey* key,
uint32_t,
Formatting.

http://codereview.chromium.org/9148006/diff/1011/src/objects.h#newcode2821
src/objects.h:2821: static inline uint32_t HashForObject(HashTableKey* key,
uint32_t,
Formatting.

http://codereview.chromium.org/9148006/diff/1011/src/objects.h#newcode2972
src/objects.h:2972: static inline uint32_t Hash(String* key, uint32_t);
Parameter should have a name.

I'm unhappy that we need a seed here, when the hash is got from the string.

http://codereview.chromium.org/9148006/diff/1011/src/objects.h#newcode2973
src/objects.h:2973: static inline uint32_t HashForObject(String* key, uint32_t,
Formatting, and the parameter should have a name.

http://codereview.chromium.org/9148006/diff/1011/src/objects.h#newcode3007
src/objects.h:3007: static inline uint32_t HashForObject(uint32_t key, uint32_t
seed,
Formatting.

http://codereview.chromium.org/9148006/diff/1011/src/objects.h#newcode3067
src/objects.h:3067: static inline uint32_t Hash(Object* key, uint32_t);
Names for arguments x2

http://codereview.chromium.org/9148006/diff/1011/src/utils.h
File src/utils.h (right):

http://codereview.chromium.org/9148006/diff/1011/src/utils.h#newcode257
src/utils.h:257: inline uint32_t ComputeIntegerHash(uint32_t key, uint32_t seed
= 0) {
Why do we need this default argument?

[indutny, 2012-01-10 06:02:39]

updated commit request.

Removed all useless arguments, added const bool variable and SeededHash method
to shape's base class (which was introduced here, btw)

If design is ok, I can start porting it to other archs.

http://codereview.chromium.org/9148006/diff/1011/src/utils.h
File src/utils.h (right):

http://codereview.chromium.org/9148006/diff/1011/src/utils.h#newcode257
src/utils.h:257: inline uint32_t ComputeIntegerHash(uint32_t key, uint32_t seed
= 0) {
Because it's used alot in other places like profiler, debugger, frames.cc . They
should not care about that.

[Erik Corry, 2012-01-10 07:29:28]

http://codereview.chromium.org/9148006/diff/8001/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/9148006/diff/8001/src/objects-inl.h#newcode2061
src/objects-inl.h:2061: HashTable<Shape, Key>::Hash(key),
This all fits on one line.

http://codereview.chromium.org/9148006/diff/8001/src/objects-inl.h#newcode4542
src/objects-inl.h:4542: return SeededHash(key, 0);
I would appreciate a comment as to why the seed is zero here.

http://codereview.chromium.org/9148006/diff/8001/src/objects-inl.h#newcode4555
src/objects-inl.h:4555: Object* other) {
Formatting

[Erik Corry, 2012-01-10 10:13:51]

http://codereview.chromium.org/9148006/diff/8001/src/ia32/macro-assembler-ia3...
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/9148006/diff/8001/src/ia32/macro-assembler-ia3...
src/ia32/macro-assembler-ia32.cc:987: // First of all lets assign to r1 value of
HashSeed
"First of all we assign the hash seed to r1."

http://codereview.chromium.org/9148006/diff/8001/src/ia32/macro-assembler-ia3...
src/ia32/macro-assembler-ia32.cc:1007: not_(r0);
The intention is that the original should be xored with the seed, but here r0 is
input unchanged into the algorithm.  In the next line half of the seed is thrown
away.

[indutny, 2012-01-10 10:17:26]

Should be fixed now. Tested in debug and release mode with snapshots and w/o.

[indutny, 2012-01-10 11:34:55]

Added test, fixed code, decoupled things :)

[indutny, 2012-01-10 11:53:02]

rebased

[Erik Corry, 2012-01-10 11:53:16]

Thanks for doing the test.

http://codereview.chromium.org/9148006/diff/9008/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/9148006/diff/9008/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:1420: mov(scratch, Operand(scratch, LSR,
kSmiTagSize));
Use SmiUntag(scratch)

http://codereview.chromium.org/9148006/diff/9008/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:1422: // Xor original key with a seed
seed -> seed.

http://codereview.chromium.org/9148006/diff/9008/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:1438: mov(scratch, Operand(2057));
This should be done with shifts and adds.  See the bleeding edge code, where I
fixed this yesterday.

http://codereview.chromium.org/9148006/diff/9008/src/ia32/macro-assembler-ia3...
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/9148006/diff/9008/src/ia32/macro-assembler-ia3...
src/ia32/macro-assembler-ia32.cc:981: xor_(r0, scratch);
You should move this xor up into the if above, because you can then do it with a
single xor instead of a mov and an xor.

http://codereview.chromium.org/9148006/diff/9008/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/9148006/diff/9008/src/objects-inl.h#newcode4539
src/objects-inl.h:4539: // This function is unreachable, since shape has
UsesSeed=true flag
Please add a period (.) at the end of all comments.

http://codereview.chromium.org/9148006/diff/9008/src/objects-inl.h#newcode4555
src/objects-inl.h:4555: uint32_t
NumberDictionaryShape::SeededHashForObject(uint32_t key, uint32_t seed,
All arguments on one line or one per line vertically aligned.

http://codereview.chromium.org/9148006/diff/9008/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/9148006/diff/9008/src/objects.h#newcode2597
src/objects.h:2597: // Won't be called if UsesSeed isn't overrided by child
class
overrided ->overridden

http://codereview.chromium.org/9148006/diff/9008/src/objects.h#newcode2602
src/objects.h:2602: // Won't be called if UsesSeed isn't overrided by child
class
And here.

http://codereview.chromium.org/9148006/diff/9008/src/objects.h#newcode2770
src/objects.h:2770: 
There's a blank line too many here.

http://codereview.chromium.org/9148006/diff/9008/src/objects.h#newcode2786
src/objects.h:2786: 
There's a missing blank line here.

http://codereview.chromium.org/9148006/diff/9008/src/objects.h#newcode3043
src/objects.h:3043: static inline uint32_t SeededHashForObject(uint32_t key,
uint32_t seed,
Formatting.

http://codereview.chromium.org/9148006/diff/9008/src/utils.h
File src/utils.h (right):

http://codereview.chromium.org/9148006/diff/9008/src/utils.h#newcode257
src/utils.h:257: inline uint32_t ComputeIntegerHash(uint32_t key, uint32_t seed
= 0) {
No default arguments.

http://codereview.chromium.org/9148006/diff/9008/test/cctest/test-hashing.cc
File test/cctest/test-hashing.cc (right):

http://codereview.chromium.org/9148006/diff/9008/test/cctest/test-hashing.cc#...
test/cctest/test-hashing.cc:237: 
Missing blank line here.

[indutny, 2012-01-10 12:30:09]

Fixed issues, fixed rebase