Improve compilation speed on a pathological case > 2x (7 sec -> 2.7 sec) by improving local lookups.

Improve compilation speed on a pathological case > 2x (7 sec -> 2.7 sec) by improving local lookups.
Committed: https://code.google.com/p/dart/source/detail?r=17085

[Ivan Posva, 2013-01-15 06:05:15]

I do not think the introduction of StringHash is necessary.

-Ivan

https://codereview.chromium.org/11893002/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11893002/diff/1/runtime/vm/object.h#newcode3903
runtime/vm/object.h:3903: virtual intptr_t Hash() const {
Can you explain to me why Hash() is a virtual in the String class? I do not see
this overriding a virtual Hash in a base class, nor are there subclasses of
String that override Hash.

[hausner, 2013-01-15 17:01:48]

The change looks functional, but I think the changes in parser.cc are awkward.

Since all names in the parser are symbols, why don't you check for identity
instead of equality?

[srdjan, 2013-01-15 18:53:24]

On 2013/01/15 17:01:48, hausner wrote:
> The change looks functional, but I think the changes in parser.cc are awkward.
> 
> Since all names in the parser are symbols, why don't you check for identity
> instead of equality?

Good suggestion, done.  This lowers compilation speed from 7 to 1.8 seconds.

[srdjan, 2013-01-15 18:58:28]

https://codereview.chromium.org/11893002/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11893002/diff/1/runtime/vm/object.h#newcode3903
runtime/vm/object.h:3903: virtual intptr_t Hash() const {
On 2013/01/15 06:05:15, Ivan Posva wrote:
> Can you explain to me why Hash() is a virtual in the String class? I do not
see
> this overriding a virtual Hash in a base class, nor are there subclasses of
> String that override Hash.

Apparently, the reason is historical. There is no need  for it being virtual.
Removed StringHash.

[hausner, 2013-01-15 19:00:22]

lgtm

https://codereview.chromium.org/11893002/diff/12001/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://codereview.chromium.org/11893002/diff/12001/runtime/vm/scopes.cc#newc...
runtime/vm/scopes.cc:300: ASSERT(var->name().IsSymbol());
Nit: instead of checking whether the names in a scope are symbols on every
lookup, we could assert that when the name is entered in the scope. We should
not make debug builds unnecessarily slow.

[srdjan, 2013-01-15 19:11:38]

https://codereview.chromium.org/11893002/diff/12001/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://codereview.chromium.org/11893002/diff/12001/runtime/vm/scopes.cc#newc...
runtime/vm/scopes.cc:300: ASSERT(var->name().IsSymbol());
On 2013/01/15 19:00:22, hausner wrote:
> Nit: instead of checking whether the names in a scope are symbols on every
> lookup, we could assert that when the name is entered in the scope. We should
> not make debug builds unnecessarily slow.

I agree that we should keep an eye on not making debug build slow. In this case
I think is needed. Assuming that the right assert is done somewhere else would
weaken the check and add a non-obvious dependency, IMHO.