Add name inference for anonymous functions to facilitate debugging and profiling of JS code.

Add name inference for anonymous functions to facilitate debugging and profiling of JS code.

Currently function name inference is wired with AST optimization pass to avoid introducing another pass over AST. A better solution would be to rewrite AST visitors so they can be naturally combined together in a single pass, as their current implementation doesn't allow it.

For examples of cases where function names can be inferred, see the tests file.

Committed: http://code.google.com/p/v8/source/detail?r=1696

[Mikhail Naganov, 2009-04-08 14:18:35]

This is an implementation of name inference for anonymous functions which I
discussed previously.

I wired my AST pass with AstOptimizer's, as Ivan suggested that I better not
introduce another AST pass. I looked over all AstVisitors we have and didn't
found a more natural place. But this forced me to make AstOptimizer running for
all functions, not only for those which have a local context.

I will update XCode project files before uploading. I just don't have a Mac at
hand.

[Mikhail Naganov, 2009-04-13 17:49:39]

On 2009/04/08 14:18:35, Mikhail Naganov wrote:
> This is an implementation of name inference for anonymous functions which I
> discussed previously.
> 
> I wired my AST pass with AstOptimizer's, as Ivan suggested that I better not
> introduce another AST pass. I looked over all AstVisitors we have and didn't
> found a more natural place. But this forced me to make AstOptimizer running
for
> all functions, not only for those which have a local context.
> 
> I will update XCode project files before uploading. I just don't have a Mac at
> hand.

Oops, I see Ivan's changes crept into objects.cc due to syncing (deltas 1 & 2.)
Please ignore them while reviewing.

[iposva, 2009-04-13 20:49:42]

I have only looked at the Xcode project. Please address the comments before
submitting.

Thanks,
-Ivan

http://codereview.chromium.org/62146/diff/4001/4020
File tools/v8.xcodeproj/project.pbxproj (right):

http://codereview.chromium.org/62146/diff/4001/4020#newcode1180
Line 1180: 9F92FAA90F8F28AD0089F02C /* func-name-inferrer.cc in Sources */,
Please keep this list sorted. If you need help just stop by my office.

http://codereview.chromium.org/62146/diff/4001/4020#newcode1282
Line 1282: 9F92FAAA0F8F28AD0089F02C /* func-name-inferrer.cc in Sources */,
ditto. This should be a sorted list.

[iposva, 2009-04-13 21:12:30]

Another comment which hopefully will make your code easier to maintain.

-Ivan

http://codereview.chromium.org/62146/diff/4001/4015
File src/rewriter.cc (right):

http://codereview.chromium.org/62146/diff/4001/4015#newcode278
Line 278: bool must_leave_inferrer = false;
It might make sense to create a stack allocated object that will automatically
call InferAndLeave() on destruction if inferring has been requested. Such as
follows:

class NeedBetterName {
 public:
  NeedBetterName(FuncNameInferrer* inferrer) : inferrer_(inferrer),
entered_(false) {};
  ~ NeedBetterName() {
    if (entered_) {
      inferrer_->InferAndLeave();
    }
  }

  void Enter() {
    inferrer_->Enter();
    entered_ = true;
  }

 private:
  FuncNameInferrer* inferrer_;
  bool entered_;
};

Which will automatically leave the inferrer. This way you are guaranteed to not
forget to call InferAndLeave() even across complicated control flow.

[Søren Thygesen Gjesse, 2009-04-13 23:50:11]

LGTM

Please update the v8_base_arm.vcproj Visual Studio project file as well.

http://codereview.chromium.org/62146/diff/4001/4008
File src/func-name-inferrer.cc (right):

http://codereview.chromium.org/62146/diff/4001/4008#newcode39
Line 39: // and starts with a capital letter.
Should we enforce the capital first letter?

http://codereview.chromium.org/62146/diff/4001/4015
File src/rewriter.cc (left):

http://codereview.chromium.org/62146/diff/4001/4015#oldcode798
Line 798: if (!scope->is_global_scope()) {
Any idea why the global scope used to be skipped here?

[Mikhail Naganov, 2009-04-13 23:51:37]

Ivan, thanks for comments!

http://codereview.chromium.org/62146/diff/4001/4015
File src/rewriter.cc (right):

http://codereview.chromium.org/62146/diff/4001/4015#newcode278
Line 278: bool must_leave_inferrer = false;
On 2009/04/13 21:12:30, iposva wrote:
> It might make sense to create a stack allocated object that will automatically
> call InferAndLeave() on destruction if inferring has been requested. Such as
> follows:
> 
> class NeedBetterName {
>  public:
>   NeedBetterName(FuncNameInferrer* inferrer) : inferrer_(inferrer),
> entered_(false) {};
>   ~ NeedBetterName() {
>     if (entered_) {
>       inferrer_->InferAndLeave();
>     }
>   }
> 
>   void Enter() {
>     inferrer_->Enter();
>     entered_ = true;
>   }
> 
>  private:
>   FuncNameInferrer* inferrer_;
>   bool entered_;
> };
> 
> Which will automatically leave the inferrer. This way you are guaranteed to
not
> forget to call InferAndLeave() even across complicated control flow.

Great advice. And thanks for the boilerplate code!

http://codereview.chromium.org/62146/diff/4001/4020
File tools/v8.xcodeproj/project.pbxproj (right):

http://codereview.chromium.org/62146/diff/4001/4020#newcode1180
Line 1180: 9F92FAA90F8F28AD0089F02C /* func-name-inferrer.cc in Sources */,
On 2009/04/13 20:49:42, iposva wrote:
> Please keep this list sorted. If you need help just stop by my office.

Done.

http://codereview.chromium.org/62146/diff/4001/4020#newcode1282
Line 1282: 9F92FAAA0F8F28AD0089F02C /* func-name-inferrer.cc in Sources */,
On 2009/04/13 20:49:42, iposva wrote:
> ditto. This should be a sorted list.

Done.

[Mikhail Naganov, 2009-04-14 00:18:30]

http://codereview.chromium.org/62146/diff/4001/4008
File src/func-name-inferrer.cc (right):

http://codereview.chromium.org/62146/diff/4001/4008#newcode39
Line 39: // and starts with a capital letter.
On 2009/04/13 23:50:11, Søren Gjesse wrote:
> Should we enforce the capital first letter?

It would make sense to add "warnings" to V8 compiler checking that constructors
(and only them) are called using the "new" operator. But I don't think that we
should enforce anything in the name inferrer.

http://codereview.chromium.org/62146/diff/4001/4015
File src/rewriter.cc (left):

http://codereview.chromium.org/62146/diff/4001/4015#oldcode798
Line 798: if (!scope->is_global_scope()) {
On 2009/04/13 23:50:11, Søren Gjesse wrote:
> Any idea why the global scope used to be skipped here?

Kevin said that "The AstOptimizer was originally intended as an analysis pass
that would allow us to make inlining decisions in generated code. Since we
didn't intend to do any inlining at the global scope anyway, there was no need."

I suggest that it makes no harm running it for the global scope, except for some
performance penalty in parsing. As I said, I'm avoiding adding another pass, and
no other AstVisitor suits better for doing name inference. Let's wait for Kasper
before submitting.

[iposva, 2009-04-14 00:36:03]

http://codereview.chromium.org/62146/diff/4001/4015
File src/rewriter.cc (left):

http://codereview.chromium.org/62146/diff/4001/4015#oldcode798
Line 798: if (!scope->is_global_scope()) {
On 2009/04/14 00:18:30, Mikhail Naganov wrote:
> On 2009/04/13 23:50:11, Søren Gjesse wrote:
> > Any idea why the global scope used to be skipped here?
> 
> Kevin said that "The AstOptimizer was originally intended as an analysis pass
> that would allow us to make inlining decisions in generated code. Since we
> didn't intend to do any inlining at the global scope anyway, there was no
need."
> 
> I suggest that it makes no harm running it for the global scope, except for
some
> performance penalty in parsing. As I said, I'm avoiding adding another pass,
and
> no other AstVisitor suits better for doing name inference. Let's wait for
Kasper
> before submitting. 

Since I put the AstOptimizer in initially here was the reasoning:
Originally the AstOptimizer was only tracking likely types through the code and
did advise the backend for which operations to emit inline Smi checks. At the
time this optimization was deemed unneeded for global code that runs only once.
We have since added additional functionality to the AstOptimizer and already
seen bugs because it was not run on global code. I strongly suggest that my
initial decision was a premature optimization and that we should always run the
AstOptimizer. When you do, please file a reminder bug for Kevin to remove his
special handling of global code in the backend. In any case you should verify
that compilation speed of global code is not impacted severely, but on the other
hand we are already running this optimization step on all other code (which is
the majority of the source in well-written code anyway.)

I don't think there is a need to wait for Kasper's input for this particular
question as he is still on holiday.