Issue 9124004: Backport hash collision workaround to 3.6.

Issue 9124004: Backport hash collision workaround to 3.6. (Closed)

Created:
8 years, 11 months ago by Erik Corry

Modified:
8 years, 11 months ago

Reviewers:
Vyacheslav Egorov (Chromium)

CC:
v8-dev

Base URL:
http://v8.googlecode.com/svn/branches/3.6/

Visibility:
Public.

Description

Backport string hash collision workaround to 3.6. This is made up of 9956, 10351, 10338, 10330 and 10362. This change bakes the string hash key into the snapshot, so it is determined at build time for shapshot configs. Committed: http://code.google.com/p/v8/source/detail?r=10363

Patch Set 1 #

Total comments: 6

Created: 8 years, 11 months ago

	Unified diffs	Side-by-side diffs	Stats (+446 lines, -162 lines)
M	src/arm/builtins-arm.cc	View	1 chunk	+1 line, -4 lines	0 comments
M	src/arm/code-stubs-arm.h	View	1 chunk	+64 lines, -0 lines	0 comments
M	src/arm/code-stubs-arm.cc	View	6 chunks	+14 lines, -72 lines	2 comments
M	src/arm/deoptimizer-arm.cc	View	1 chunk	+1 line, -3 lines	0 comments
M	src/arm/macro-assembler-arm.h	View	2 chunks	+7 lines, -1 line	0 comments
M	src/arm/macro-assembler-arm.cc	View	1 chunk	+2 lines, -2 lines	0 comments
M	src/d8.cc	View	1 chunk	+6 lines, -1 line	0 comments
M	src/flag-definitions.h	View	1 chunk	+8 lines, -0 lines	0 comments
M	src/heap.h	View	3 chunks	+8 lines, -2 lines	0 comments
M	src/heap.cc	View	1 chunk	+11 lines, -0 lines	0 comments
M	src/ia32/code-stubs-ia32.cc	View	6 chunks	+26 lines, -9 lines	0 comments
M	src/ia32/macro-assembler-ia32.cc	View	1 chunk	+0 lines, -2 lines	0 comments
M	src/mips/code-stubs-mips.cc	View	2 chunks	+10 lines, -6 lines	2 comments
M	src/mips/macro-assembler-mips.h	View	1 chunk	+5 lines, -0 lines	0 comments
M	src/objects.h	View	4 chunks	+10 lines, -3 lines	2 comments
M	src/objects.cc	View	14 chunks	+54 lines, -28 lines	0 comments
M	src/objects-inl.h	View	4 chunks	+11 lines, -8 lines	0 comments
M	src/profile-generator.cc	View	3 chunks	+7 lines, -3 lines	0 comments
M	src/x64/code-stubs-x64.cc	View	6 chunks	+15 lines, -9 lines	0 comments
M	test/cctest/SConscript	View	1 chunk	+1 line, -0 lines	0 comments
A	test/cctest/test-hashing.cc	View	1 chunk	+172 lines, -0 lines	0 comments
M	test/mjsunit/debug-evaluate-locals-optimized.js	View	2 chunks	+7 lines, -5 lines	0 comments
M	test/mjsunit/debug-evaluate-locals-optimized-double.js	View	1 chunk	+6 lines, -4 lines	0 comments

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Vyacheslav Egorov (Chromium)

lgtm http://codereview.chromium.org/9124004/diff/1/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right): http://codereview.chromium.org/9124004/diff/1/src/arm/code-stubs-arm.cc#newcode5398 src/arm/code-stubs-arm.cc:5398: __ and_(hash, hash, Operand(kHashShiftCutOffMask)); Shouldn't it be SetCC? ...

8 years, 11 months ago (2012-01-09 19:09:33 UTC) #2

Erik Corry

8 years, 11 months ago (2012-01-10 00:18:44 UTC) #3

http://codereview.chromium.org/9124004/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/9124004/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:5398: __ and_(hash, hash,
Operand(kHashShiftCutOffMask));
On 2012/01/09 19:09:33, Vyacheslav Egorov wrote:
> Shouldn't it be SetCC? (instead of SetCC on the one above).

Good catch, this needs fixing in bleeding edge too.

http://codereview.chromium.org/9124004/diff/1/src/mips/code-stubs-mips.cc
File src/mips/code-stubs-mips.cc (right):

http://codereview.chromium.org/9124004/diff/1/src/mips/code-stubs-mips.cc#new...
src/mips/code-stubs-mips.cc:5621: // if (hash == 0) hash = 27;
On 2012/01/09 19:09:33, Vyacheslav Egorov wrote:
> should not there be some special masking here?

As noted in the description I wasn't able to get MIPS working correctly, so
there may be a followup patch for MIPS (it was already broken in this branch if
I understand correctly, the external MIPS git repo is apparently the place to go
for 3.6).

http://codereview.chromium.org/9124004/diff/1/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/9124004/diff/1/src/objects.h#newcode6080
src/objects.h:6080: static const uint32_t kHashBitMask = 0xffffffffu >>
kHashShift;
On 2012/01/09 19:09:33, Vyacheslav Egorov wrote:
> this constant is duplicated in every code generator.

Yes, this should be fixed in bleeding edge.

Expand Messages | Collapse Messages