Extract hardcoded error strings into a single place and replace them with enum.

Extract hardcoded error strings into a single place and replace them with enum.

I'd like to propagate bailout reason to cpu profiler.
So I need to save it into heap object SharedFunctionInfo.
But:
1) all bailout reason strings spread across all the sources.
2) they are native strings and if I convert them into String then I may have a performance issue.
3) one byte is enough for 184 bailout reasons. Otherwise we need 8 bytes for the pointer.

Also I think it would be nice to have error strings collected in one place.
In that case we will get additional benefits:

It allows us to keep this set of messages under control.
It gives us a chance to internationalize them.
It slightly reduces the binary footprint. 

From the other hand the developers have to add new strings into that enum.

BUG=
R=jkummerow@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=16024

[loislo, 2013-08-01 09:52:04]

the most part of the patch is a mechanical changes that were made by the script.




use strict;
my %map;
my $filter =
'Bailout\("|Abort\("|set_bailout_reason\("|DisableOptimization\("|\sCheck\([^"].+"|\sAssert\([^"].+"';
print $filter . "\n";
my @reasons = `git grep -E '$filter' | cut -f2 -d\\" | sort | uniq`;

foreach my $string (@reasons) {
  chomp $string;
  my $constant = $string;
  $constant =~ s/((^\w)|\W(\w))/\U$1/g;
  $constant =~ s/\W//g;
  $constant = "k" . $constant;
  $map{$string} = $constant;
  print $string . " => " . $constant . "\n";
}

my @matched_files = `git grep -lE '$filter'`;
foreach my $file (@matched_files) {
  chomp $file;
  print $file . "...";
  foreach my $string (keys %map) {
    $string =~ s/\//\\\//; # escape slash symbol.
    print `sed -i "s/\\"$string\\"/$map{$string}/" $file`;
  }
  print "done.\n";
}

print "enum BailoutReason {\n";
foreach my $reason (@reasons) {
  print "  " . $map{$reason} . ",\n";
}
print "}\n";

print "\n\n\n";

print "const char* GetBailoutReason(BailoutReason reason) {\n";
print "  switch (reason) {\n";
foreach my $reason (@reasons) {
  print "    case " . $map{$reason} . ": return \"" . $reason . "\";\n";
}
print "  }\n";
print "}\n";

[alph, 2013-08-01 10:35:50]

https://codereview.chromium.org/20843012/diff/6001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/20843012/diff/6001/src/objects.cc#newcode15968
src/objects.cc:15968: ASSERT(reason >= kLastErrorMessage);
<

https://codereview.chromium.org/20843012/diff/6001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/20843012/diff/6001/src/objects.h#newcode1123
src/objects.h:1123:
V(kInlinedRuntimeFunctionIsStringWrapperSafeForDefaultValueOf, "inlined runtime
function: IsStringWrapperSafeForDefaultValueOf") \
80 chars

[Jakob Kummerow, 2013-08-01 12:02:13]

DBC.

Could you explain briefly what the motivation for this change is?

https://codereview.chromium.org/20843012/diff/6001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/20843012/diff/6001/src/compiler.cc#newcode488
src/compiler.cc:488: info()->set_bailout_reason(kCodeGenerationFailed);
While you're here, please wrap this into an "if (info()->bailout_reason() !=
kNoReason) {...}" check (otherwise it overwrites the more detailed reason that
the Lithium code generator might have set).

https://codereview.chromium.org/20843012/diff/6001/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

https://codereview.chromium.org/20843012/diff/6001/src/ia32/stub-cache-ia32.c...
src/ia32/stub-cache-ia32.cc:3156: __ Check(not_equal,
kDontDeleteCellsCanTContainTheHole);
nit: s/CanT/Cannot/

https://codereview.chromium.org/20843012/diff/6001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/20843012/diff/6001/src/objects.cc#newcode15970
src/objects.cc:15970: static const char* error_messages_[] =
{ERROR_MESSAGES_LIST(ERROR_MESSAGES_TEXTS)};
nit: 80col

[loislo, 2013-08-01 12:27:59]

On 2013/08/01 12:02:13, Jakob wrote:
> DBC.
> 
> Could you explain briefly what the motivation for this change is?
> 
> https://codereview.chromium.org/20843012/diff/6001/src/compiler.cc
> File src/compiler.cc (right):
> 
> https://codereview.chromium.org/20843012/diff/6001/src/compiler.cc#newcode488
> src/compiler.cc:488: info()->set_bailout_reason(kCodeGenerationFailed);
> While you're here, please wrap this into an "if (info()->bailout_reason() !=
> kNoReason) {...}" check (otherwise it overwrites the more detailed reason that
> the Lithium code generator might have set).
> 
> https://codereview.chromium.org/20843012/diff/6001/src/ia32/stub-cache-ia32.cc
> File src/ia32/stub-cache-ia32.cc (right):
> 
>
https://codereview.chromium.org/20843012/diff/6001/src/ia32/stub-cache-ia32.c...
> src/ia32/stub-cache-ia32.cc:3156: __ Check(not_equal,
> kDontDeleteCellsCanTContainTheHole);
> nit: s/CanT/Cannot/
> 
> https://codereview.chromium.org/20843012/diff/6001/src/objects.cc
> File src/objects.cc (right):
> 
> https://codereview.chromium.org/20843012/diff/6001/src/objects.cc#newcode15970
> src/objects.cc:15970: static const char* error_messages_[] =
> {ERROR_MESSAGES_LIST(ERROR_MESSAGES_TEXTS)};
> nit: 80col

In cpu profiler we are showing the times for a function.
Sometimes the function needs unacceptable amount of time.
It might happen if the function was deoptimized for some reason.
I'd like to expose that fact in the cpu-profile and show to the web developer
the reason of that. Otherwise they can detect that only with help of js-flags
magic.

It happens that we have the reason only in CompilationInfo which has
limited lifetime. So I'd like to propagate the reason to SharedFunctionInfo as a
property.
But SharedFunctionInfo is a heap object, so I have to convert the reason into
String.
That may introduce unnecessary overhead in the code and affects the performance.

Yesterday I was optimistic and thought that we have a handful of reasons.
So I decided to use an enum for that and save it into SharedFunctionInfo.
Today morning I found that we have a little bit more reasons (184).
But from the other hand I found that all these strings actually are error
messages.

I think it would be nice to have them collected in one place.
It allows us to keep this set of messages under control.
It reduces the binary footprint.
It gives us a chance to internationalize them.

[loislo, 2013-08-01 13:15:29]

https://codereview.chromium.org/20843012/diff/6001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/20843012/diff/6001/src/compiler.cc#newcode488
src/compiler.cc:488: info()->set_bailout_reason(kCodeGenerationFailed);
On 2013/08/01 12:02:13, Jakob wrote:
> While you're here, please wrap this into an "if (info()->bailout_reason() !=
> kNoReason) {...}" check (otherwise it overwrites the more detailed reason that
> the Lithium code generator might have set).

Done.

https://codereview.chromium.org/20843012/diff/6001/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

https://codereview.chromium.org/20843012/diff/6001/src/ia32/stub-cache-ia32.c...
src/ia32/stub-cache-ia32.cc:3156: __ Check(not_equal,
kDontDeleteCellsCanTContainTheHole);
On 2013/08/01 12:02:13, Jakob wrote:
> nit: s/CanT/Cannot/

Done.

https://codereview.chromium.org/20843012/diff/6001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/20843012/diff/6001/src/objects.cc#newcode15968
src/objects.cc:15968: ASSERT(reason >= kLastErrorMessage);
On 2013/08/01 10:35:50, alph wrote:
> <

Done.

https://codereview.chromium.org/20843012/diff/6001/src/objects.cc#newcode15970
src/objects.cc:15970: static const char* error_messages_[] =
{ERROR_MESSAGES_LIST(ERROR_MESSAGES_TEXTS)};
On 2013/08/01 12:02:13, Jakob wrote:
> nit: 80col

Done.

https://codereview.chromium.org/20843012/diff/6001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/20843012/diff/6001/src/objects.h#newcode1123
src/objects.h:1123:
V(kInlinedRuntimeFunctionIsStringWrapperSafeForDefaultValueOf, "inlined runtime
function: IsStringWrapperSafeForDefaultValueOf") \
On 2013/08/01 10:35:50, alph wrote:
> 80 chars

Done.

[Jakob Kummerow, 2013-08-02 08:13:46]

LGTM with one more comment.

https://codereview.chromium.org/20843012/diff/16001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/20843012/diff/16001/src/compiler.cc#newcode488
src/compiler.cc:488: if (info()->bailout_reason() != kNoReason) {
Sorry, I mixed up the condition in my earlier comment. Obviously this should be
"==" -- only set the reason if it's not set yet.

[loislo, 2013-08-02 09:53:43]

Committed patchset #4 manually as r16024 (presubmit successful).

[loislo, 2013-08-02 09:54:51]

On 2013/08/02 08:13:46, Jakob wrote:
> LGTM with one more comment.
> 
> https://codereview.chromium.org/20843012/diff/16001/src/compiler.cc
> File src/compiler.cc (right):
> 
> https://codereview.chromium.org/20843012/diff/16001/src/compiler.cc#newcode488
> src/compiler.cc:488: if (info()->bailout_reason() != kNoReason) {
> Sorry, I mixed up the condition in my earlier comment. Obviously this should
be
> "==" -- only set the reason if it's not set yet.

done

[Jakob Kummerow, 2013-08-09 14:02:29]

On 2013/08/02 09:54:51, loislo wrote:
> On 2013/08/02 08:13:46, Jakob wrote:
> > LGTM with one more comment.
> > 
> > https://codereview.chromium.org/20843012/diff/16001/src/compiler.cc
> > File src/compiler.cc (right):
> > 
> >
https://codereview.chromium.org/20843012/diff/16001/src/compiler.cc#newcode488
> > src/compiler.cc:488: if (info()->bailout_reason() != kNoReason) {
> > Sorry, I mixed up the condition in my earlier comment. Obviously this should
> be
> > "==" -- only set the reason if it's not set yet.
> 
> done

I think you forgot to commit this change or something. bleeding_edge still says:

      if (info()->bailout_reason() != kNoReason) {
        info()->set_bailout_reason(kCodeGenerationFailed);
      }

which is bogus.

[yurys, 2013-08-09 15:09:26]

On 2013/08/09 14:02:29, Jakob wrote:
> On 2013/08/02 09:54:51, loislo wrote:
> > On 2013/08/02 08:13:46, Jakob wrote:
> > > LGTM with one more comment.
> > > 
> > > https://codereview.chromium.org/20843012/diff/16001/src/compiler.cc
> > > File src/compiler.cc (right):
> > > 
> > >
> https://codereview.chromium.org/20843012/diff/16001/src/compiler.cc#newcode488
> > > src/compiler.cc:488: if (info()->bailout_reason() != kNoReason) {
> > > Sorry, I mixed up the condition in my earlier comment. Obviously this
should
> > be
> > > "==" -- only set the reason if it's not set yet.
> > 
> > done
> 
> I think you forgot to commit this change or something. bleeding_edge still
says:
> 
>       if (info()->bailout_reason() != kNoReason) {
>         info()->set_bailout_reason(kCodeGenerationFailed);
>       }
> 
> which is bogus.

Fix is up for review: https://codereview.chromium.org/22404007/