Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1)

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

Messages

Total messages: 3 (0 generated)
Erik Corry
8 years, 11 months ago (2012-01-06 15:22:40 UTC) #1
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.

Powered by Google App Engine
This is Rietveld 408576698