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

Issue 19749004: With >= 64 non-string instance types, I removed an optimization (Closed)

Created:
7 years, 5 months ago by mvstanton
Modified:
7 years, 5 months ago
Reviewers:
Yang
CC:
v8-dev
Visibility:
Public.

Description

With >= 64 non-string instance types, I removed an optimization that allowed us to detect internalized strings with a single bit test. (https://code.google.com/p/v8/source/detail?r=15358) But that change caused a regression, so here is an improvement: Put the internalized string types in the lower 64 entries of INSTANCE_TYPE, and non-internalized string types in the next 64 entries. This way we can restore the single bit check. BUG= R=yangguo@chromium.org Committed: https://code.google.com/p/v8/source/detail?r=15773

Patch Set 1 : A couple fixes #

Total comments: 7

Patch Set 2 : Review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+129 lines, -125 lines) Patch
M src/arm/code-stubs-arm.cc View 5 chunks +16 lines, -29 lines 0 comments Download
M src/arm/ic-arm.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M src/arm/macro-assembler-arm.cc View 1 chunk +7 lines, -4 lines 0 comments Download
M src/hydrogen-instructions.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/ia32/code-stubs-ia32.cc View 1 4 chunks +11 lines, -16 lines 0 comments Download
M src/ia32/ic-ia32.cc View 1 chunk +4 lines, -3 lines 0 comments Download
M src/ia32/macro-assembler-ia32.cc View 1 chunk +8 lines, -5 lines 0 comments Download
M src/objects.h View 1 2 chunks +50 lines, -33 lines 0 comments Download
M src/objects-inl.h View 1 2 chunks +6 lines, -6 lines 0 comments Download
M src/x64/code-stubs-x64.cc View 4 chunks +11 lines, -17 lines 0 comments Download
M src/x64/ic-x64.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M src/x64/macro-assembler-x64.cc View 1 chunk +9 lines, -5 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
mvstanton
Hi Yang, Here is the internalized string instance type change we discussed. Thanks for having ...
7 years, 5 months ago (2013-07-19 09:20:29 UTC) #1
Yang
lgtm with comment. https://codereview.chromium.org/19749004/diff/2001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/19749004/diff/2001/src/objects.h#newcode640 src/objects.h:640: INTERNALIZED_STRING_TYPE = kTwoByteStringTag | kSeqStringTag, I ...
7 years, 5 months ago (2013-07-19 09:45:06 UTC) #2
Yang
https://codereview.chromium.org/19749004/diff/2001/src/ia32/code-stubs-ia32.cc File src/ia32/code-stubs-ia32.cc (right): https://codereview.chromium.org/19749004/diff/2001/src/ia32/code-stubs-ia32.cc#newcode4116 src/ia32/code-stubs-ia32.cc:4116: __ cmp(scratch, kStringTag | kInternalizedTag); change that to a ...
7 years, 5 months ago (2013-07-19 09:52:18 UTC) #3
mvstanton
Thanks for the review Yang, I updated per your points. All the best, --Michael https://codereview.chromium.org/19749004/diff/2001/src/ia32/code-stubs-ia32.cc ...
7 years, 5 months ago (2013-07-19 11:07:41 UTC) #4
mvstanton
7 years, 5 months ago (2013-07-19 11:29:20 UTC) #5
Message was sent while issue was closed.
Committed patchset #2 manually as r15773 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698