Rewrite Zone implementation to avoid recursively looking up stuff in the parent chain.

Rewrite Zone implementation to avoid recursively looking up stuff in the parent chain.

Reuses as much as possible of the parent zone's implementation representation.

Increases performance of primitive tests of only async operations by ~10% (but with a lot of variability and a spurious regression).

R=floitsch@google.com

Committed: https://code.google.com/p/dart/source/detail?r=37807

[Anders Johnsen, 2014-06-24 12:21:59]

Can you add some numbers for code-size (simple program using a custom zone, I
guess).

[floitsch, 2014-06-24 12:50:30]

LGTM but wait for Anders.

https://codereview.chromium.org/331993005/diff/1/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/331993005/diff/1/sdk/lib/async/zone.dart#newc...
sdk/lib/async/zone.dart:703: // We do not optimizie for repeatedly looking up a
key which isn't
optimize

https://codereview.chromium.org/331993005/diff/1/sdk/lib/async/zone.dart#newc...
sdk/lib/async/zone.dart:928: valueMap = zone._map;
clever.

https://codereview.chromium.org/331993005/diff/1/sdk/lib/async/zone.dart#newc...
sdk/lib/async/zone.dart:957: class _RootZoneDelegate implements ZoneDelegate {
I wonder if this will cost more than it is worth.
More code, more polymorphic call-sites, ...
Can't we use the normal _ZoneDelegate (filled with corresponding _ZoneFunctions)
instead?

https://codereview.chromium.org/331993005/diff/1/sdk/lib/async/zone.dart#newc...
sdk/lib/async/zone.dart:987: class _RootZone extends _Zone {
Ditto: can't we use the normal _CustomZone to minimize size and polymorphism?

https://codereview.chromium.org/331993005/diff/1/sdk/lib/async/zone.dart#newc...
sdk/lib/async/zone.dart:1098: _rootHandleUncaughtError(null, null, this, error,
stackTrace);
return

Yes: I think there are cases where handleUncaughtError may return.
Unfortunately I don't remember :(

[Lasse Reichstein Nielsen, 2014-06-24 14:56:06]

https://codereview.chromium.org/331993005/diff/1/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/331993005/diff/1/sdk/lib/async/zone.dart#newc...
sdk/lib/async/zone.dart:957: class _RootZoneDelegate implements ZoneDelegate {
I can try with a normal _ZoneDelegate for the root zone.

I tried making the root zone itself a customzone, but it ended up being a 10%
regression instead of an improvement.

Sadly this seems like this might be a space/time tradeoff. I get 15k more code
from dart2js on a unittest that uses a custom zone.

https://codereview.chromium.org/331993005/diff/1/sdk/lib/async/zone.dart#newc...
sdk/lib/async/zone.dart:987: class _RootZone extends _Zone {
No, I tried that and it was signficantly less efficient.

https://codereview.chromium.org/331993005/diff/1/sdk/lib/async/zone.dart#newc...
sdk/lib/async/zone.dart:1098: _rootHandleUncaughtError(null, null, this, error,
stackTrace);
Ack. I was wondering about the return type.

[Anders Johnsen, 2014-06-25 09:32:28]

On 2014/06/24 14:56:06, Lasse Reichstein Nielsen wrote:
> https://codereview.chromium.org/331993005/diff/1/sdk/lib/async/zone.dart
> File sdk/lib/async/zone.dart (right):
> 
>
https://codereview.chromium.org/331993005/diff/1/sdk/lib/async/zone.dart#newc...
> sdk/lib/async/zone.dart:957: class _RootZoneDelegate implements ZoneDelegate {
> I can try with a normal _ZoneDelegate for the root zone.
> 
> I tried making the root zone itself a customzone, but it ended up being a 10%
> regression instead of an improvement.
> 
> Sadly this seems like this might be a space/time tradeoff. I get 15k more code
> from dart2js on a unittest that uses a custom zone.

So the current CL is 15k more code when using zones?

- Anders
> 
>
https://codereview.chromium.org/331993005/diff/1/sdk/lib/async/zone.dart#newc...
> sdk/lib/async/zone.dart:987: class _RootZone extends _Zone {
> No, I tried that and it was signficantly less efficient.
> 
>
https://codereview.chromium.org/331993005/diff/1/sdk/lib/async/zone.dart#newc...
> sdk/lib/async/zone.dart:1098: _rootHandleUncaughtError(null, null, this,
error,
> stackTrace);
> Ack. I was wondering about the return type.

[Lasse Reichstein Nielsen, 2014-06-25 11:20:58]

Address comments

[Lasse Reichstein Nielsen, 2014-06-30 07:25:19]

Ok, I'll try committing this to see how much it affects size in real examples.

[Lasse Reichstein Nielsen, 2014-06-30 07:29:06]

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