Externalize deoptimization reasons.

Externalize deoptimization reasons.

1) The hardcoded strings were converted into DeoptReason enum.

2) Deopt comment were converted into a pair location and deopt reason entries so
the deopt reason tracking mode would less affect the size of the RelocInfo table and heap.

3) DeoptReason entry in RelocInfo reuses kCommentTag value and generates short entry in RelocInfo table.

BUG=452067
LOG=n

Committed: https://crrev.com/c49820e45b57f128a98690940875c049f612dde6
Cr-Commit-Position: refs/heads/master@{#26434}

Committed: https://crrev.com/ec42e002da03adb2db968dd5b7453341ddc59a5c
Cr-Commit-Position: refs/heads/master@{#26448}

Committed: https://crrev.com/2491a639bf46da4bfdcf65329305ee3053aa5fec
Cr-Commit-Position: refs/heads/master@{#26463}

[loislo, 2015-01-26 17:55:57]

loislo@chromium.org changed reviewers:
+ alph@chromium.org, svenpanne@chromium.org, yurys@chromium.org

[Sven Panne, 2015-01-29 11:32:51]

Looking good so far. Apart from the missing platform ports, it should be
functionally equivalent to the current code, correct? (Well, you don't even need
--code-comments anymore, I guess)

https://codereview.chromium.org/874323003/diff/1/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/874323003/diff/1/src/deoptimizer.cc#newcode3636
src/deoptimizer.cc:3636: DEOPT_MESSAGES_LIST(DEOPT_MESSAGES_TEXTS)};
Nit: Move the "};" to a separate line.

https://codereview.chromium.org/874323003/diff/1/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/874323003/diff/1/src/ia32/assembler-ia32.h#ne...
src/ia32/assembler-ia32.h:1267: bool force = false);
Do we really need the "force" flag?

[Sven Panne, 2015-01-30 11:50:08]

2 remarks after talking about this at lunch... :-)

https://codereview.chromium.org/874323003/diff/1/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/874323003/diff/1/src/assembler.h#newcode376
src/assembler.h:376: DEOPT_REASON,        // Deoptimization reason index.
Ooops, I just noticed that this might not work: We are probably running out of
bits for the reason. You should have a closer look at that, perhaps I'm wrong...

Another thing to watch out for is performance: The encoding of the RelocInfo is
very sensitive to minor changes, and mvstanton@ had problems with this in the
past. Perhaps that's a non-issue here (if we only emit the new kind of infos
with --trace-deopt), but nevertheless something to watch out for (and measure).

[loislo, 2015-01-30 13:17:47]

https://codereview.chromium.org/874323003/diff/1/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/874323003/diff/1/src/assembler.h#newcode376
src/assembler.h:376: DEOPT_REASON,        // Deoptimization reason index.
On 2015/01/30 at 11:50:08, Sven Panne wrote:
> Ooops, I just noticed that this might not work: We are probably running out of
bits for the reason. You should have a closer look at that, perhaps I'm wrong...
> 
> Another thing to watch out for is performance: The encoding of the RelocInfo
is very sensitive to minor changes, and mvstanton@ had problems with this in the
past. Perhaps that's a non-issue here (if we only emit the new kind of infos
with --trace-deopt), but nevertheless something to watch out for (and measure).

I did an investigation and found that we have about 60 different reasons for
deopt at the moment across all the platforms. So we fit into 6 bit of short
version (2 bytes) of RelocInfo. The overall encoding schema for 1 byte and 2
bytes RelocInfo looks reasonable but the layout for the long version looks a bit
strange and could not be extended. So I'm thinking how it could be redesigned.
Fortunately it could be done as a separate patch with proper performance
regression testing.

[Sven Panne, 2015-01-30 13:25:47]

https://codereview.chromium.org/874323003/diff/1/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/874323003/diff/1/src/assembler.h#newcode376
src/assembler.h:376: DEOPT_REASON,        // Deoptimization reason index.
I was more concerned about encoding the Mode. Adding Michael...

[loislo, 2015-02-04 07:53:44]

loislo@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[loislo, 2015-02-04 07:54:56]

Michael, could you please take a look.

The patch is working for x64, ia32, mips, mips64, arm and arm64

https://codereview.chromium.org/874323003/diff/1/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/874323003/diff/1/src/assembler.h#newcode376
src/assembler.h:376: DEOPT_REASON,        // Deoptimization reason index.
On 2015/01/30 at 13:25:47, Sven Panne wrote:
> I was more concerned about encoding the Mode. Adding Michael...

I used short encoding and reused b11 as the short mode id. It looks safe because
we don't use kComment for the short encoding but might be I missed something.

[Michael Starzinger, 2015-02-04 11:30:52]

LGTM with comments. I mainly looked at the RelocInfo part, only glimpsed over
the Hydrogen and Lithium changes.

https://codereview.chromium.org/874323003/diff/60001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/874323003/diff/60001/src/assembler.cc#newcode331
src/assembler.cc:331: // Reuse the same value for deopt reason tag in short
record format.
IIUC, this assumes that comments are always encoded in "long format" and deopt
reasons are always encoded in "short format". Correct? Can we make this explicit
in this comment?

https://codereview.chromium.org/874323003/diff/60001/src/assembler.cc#newcode429
src/assembler.cc:429: if (rmode == RelocInfo::DEOPT_REASON) {
nit: Can we move this to after EMBEDDED_OBJECT, CODE_TARGET, and
CODE_TARGET_WITH_ID, just so that the comment talking about the "two most common
modes" is still accurate and the two most common modes really are handled first.

https://codereview.chromium.org/874323003/diff/60001/src/assembler.cc#newcode431
src/assembler.cc:431: WriteTaggedData(rinfo->data(), kDeoptReasonTag);
Can we have a DCHECK here that the deopt reason really fits into kSmallDataBits?

https://codereview.chromium.org/874323003/diff/60001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/874323003/diff/60001/src/ia32/assembler-ia32....
src/ia32/assembler-ia32.h:1264: // Use --trace-deopt to enable, or provide
"force = true" flag to always
nit: There doesn't seem to be a "force" flag, let's drop that part of the
comment.

https://codereview.chromium.org/874323003/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/874323003/diff/60001/src/objects.cc#newcode10552
src/objects.cc:10552: RelocInfo::ModeMask(RelocInfo::DEOPT_REASON) |
This mode doesn't seem to be handled below explicitly and it doesn't seem to be
part of RelocInfo::apply, does it really need to be part of the mask here?

[Sven Panne, 2015-02-04 11:48:16]

LGTM from my side, too, when Michael's comments are addressed. Furthermore,
please rebase + restart the try bots.

[loislo, 2015-02-04 14:37:59]

New patchsets have been uploaded after l-g-t-m from
mstarzinger@chromium.org,svenpanne@chromium.org

[loislo, 2015-02-04 14:38:50]

comments addressed

https://codereview.chromium.org/874323003/diff/60001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/874323003/diff/60001/src/assembler.cc#newcode331
src/assembler.cc:331: // Reuse the same value for deopt reason tag in short
record format.
On 2015/02/04 at 11:30:51, Michael Starzinger wrote:
> IIUC, this assumes that comments are always encoded in "long format" and deopt
reasons are always encoded in "short format". Correct? Can we make this explicit
in this comment?

done

https://codereview.chromium.org/874323003/diff/60001/src/assembler.cc#newcode429
src/assembler.cc:429: if (rmode == RelocInfo::DEOPT_REASON) {
On 2015/02/04 at 11:30:51, Michael Starzinger wrote:
> nit: Can we move this to after EMBEDDED_OBJECT, CODE_TARGET, and
CODE_TARGET_WITH_ID, just so that the comment talking about the "two most common
modes" is still accurate and the two most common modes really are handled first.

done

https://codereview.chromium.org/874323003/diff/60001/src/assembler.cc#newcode431
src/assembler.cc:431: WriteTaggedData(rinfo->data(), kDeoptReasonTag);
On 2015/02/04 at 11:30:51, Michael Starzinger wrote:
> Can we have a DCHECK here that the deopt reason really fits into
kSmallDataBits?

done

https://codereview.chromium.org/874323003/diff/60001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/874323003/diff/60001/src/ia32/assembler-ia32....
src/ia32/assembler-ia32.h:1264: // Use --trace-deopt to enable, or provide
"force = true" flag to always
On 2015/02/04 at 11:30:51, Michael Starzinger wrote:
> nit: There doesn't seem to be a "force" flag, let's drop that part of the
comment.

done

https://codereview.chromium.org/874323003/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/874323003/diff/60001/src/objects.cc#newcode10552
src/objects.cc:10552: RelocInfo::ModeMask(RelocInfo::DEOPT_REASON) |
On 2015/02/04 at 11:30:51, Michael Starzinger wrote:
> This mode doesn't seem to be handled below explicitly and it doesn't seem to
be part of RelocInfo::apply, does it really need to be part of the mask here?

done

[loislo, 2015-02-04 16:34:22]

The CQ bit was checked by loislo@chromium.org

[commit-bot: I haz the power, 2015-02-04 16:35:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/874323003/100001

[commit-bot: I haz the power, 2015-02-04 16:35:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-04 16:35:37]

Patchset 6 (id:??) landed as
https://crrev.com/c49820e45b57f128a98690940875c049f612dde6
Cr-Commit-Position: refs/heads/master@{#26434}

[loislo, 2015-02-04 16:47:38]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/892843007/ by loislo@chromium.org.

The reason for reverting is: it broke the build.

[loislo, 2015-02-05 06:23:57]

The CQ bit was checked by loislo@chromium.org

[commit-bot: I haz the power, 2015-02-05 06:24:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/874323003/120001

[commit-bot: I haz the power, 2015-02-05 06:25:08]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-05 06:25:32]

Patchset 7 (id:??) landed as
https://crrev.com/ec42e002da03adb2db968dd5b7453341ddc59a5c
Cr-Commit-Position: refs/heads/master@{#26448}

[Benedikt Meurer, 2015-02-05 12:01:59]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/900223002/ by bmeurer@chromium.org.

The reason for reverting is: This CL breaks RelocInfo::INTERNAL_REFERENCE (and
maybe others that come after DEOPT_REASON), which is currently (mostly) unused
on tip-of-tree, but will be used for jump tables soon(ish) and should therefore
work. It seems to be a problem with implicitly assumptions about the number of
reloc info modes. Needs further investigation..

[loislo, 2015-02-05 13:13:36]

loislo@chromium.org changed reviewers:
+ bmeurer@chromium.org

[loislo, 2015-02-05 13:13:37]

Hi Benedikt, I've found the root problem but it was too late :) 

I fixed the DCHECK in RelocInfoWriter::Write last 'else' section and moved
DEOPT_REASON down. 

could you please take a look.

[Michael Starzinger, 2015-02-05 13:38:26]

LGTM (rubber-stamped).

[loislo, 2015-02-05 13:39:31]

The CQ bit was checked by loislo@chromium.org

[commit-bot: I haz the power, 2015-02-05 13:40:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/874323003/160001

[commit-bot: I haz the power, 2015-02-05 14:51:53]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-02-05 14:52:08]

Patchset 9 (id:??) landed as
https://crrev.com/2491a639bf46da4bfdcf65329305ee3053aa5fec
Cr-Commit-Position: refs/heads/master@{#26463}

[dcarney, 2015-02-11 14:34:54]

this CL seems to have broken --trace-deopt on arm64, maybe in general, but in
particular when executing the following on ToT:

./out/arm64.debug/d8 --test --random-seed=-235865360 --turbo-deoptimization
--turbo-filter=* --always-opt --debug-code --verify-heap --gc-interval=500
--stress-compaction test/mjsunit/mjsunit.js
test/mjsunit/regress/regress-builtinbust-7.js --trace-deopt

the cl is currently unrevertable in any kind of simple way, please fix!

[loislo, 2015-02-11 14:36:21]

On 2015/02/11 14:34:54, dcarney wrote:
> this CL seems to have broken --trace-deopt on arm64, maybe in general, but in
> particular when executing the following on ToT:
> 
> ./out/arm64.debug/d8 --test --random-seed=-235865360 --turbo-deoptimization
> --turbo-filter=* --always-opt --debug-code --verify-heap --gc-interval=500
> --stress-compaction test/mjsunit/mjsunit.js
> test/mjsunit/regress/regress-builtinbust-7.js --trace-deopt
> 
> the cl is currently unrevertable in any kind of simple way, please fix!

I'll take a look.