Various caches for incremental compilation.

Various caches for incremental compilation.

R=johnniwinther@google.com

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

[ahe, 2014-06-18 13:15:21]

The good stuff ;-)

[Johnni Winther, 2014-06-19 09:22:32]

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
File dart/sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
dart/sdk/lib/_internal/compiler/implementation/enqueue.dart:724: final
Set<Element> newlyEnqueuedElements = new HashSet<Element>.identity();
Element equality is object identity so why do you need HashSet.identity ?

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
File
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart
(right):

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
dart/sdk/lib/_internal/compiler/implementation/js_backend/native_emitter.dart:10:
new HashMap<Element, ClassBuilder>.identity();
Ditto.

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart
(right):

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:21:
final Set<Constant> cachedEmittedConstants = new Set<Constant>.identity();
We canonicalize these so why use identity?

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1800:
// print('Invalidating ${element.enclosingClass} due to ${element}');
Remove debug line.

[ahe, 2014-06-19 09:36:19]

Thank you for taking a look, Johnni!

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
File dart/sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
dart/sdk/lib/_internal/compiler/implementation/enqueue.dart:724: final
Set<Element> newlyEnqueuedElements = new HashSet<Element>.identity();
On 2014/06/19 09:22:32, Johnni Winther wrote:
> Element equality is object identity so why do you need HashSet.identity ? 

Good point. I'll investigate if it matters. I doubt it will for elements.

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart
(right):

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:21:
final Set<Constant> cachedEmittedConstants = new Set<Constant>.identity();
On 2014/06/19 09:22:32, Johnni Winther wrote:
> We canonicalize these so why use identity?

Think of it this way: the way we implement canonicalization is via operator==
and hashCode on constants. So every time I add a constant to this set, it gets
canonicalized again. That's expensive.

Since the constants are already canonicalized, using identity saves the second
canonicalization which is expensive for maps and lists. So much that it cost 1-2
seconds for a small example which uses package:intl.

[floitsch, 2014-06-19 12:00:24]

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
File dart/sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
dart/sdk/lib/_internal/compiler/implementation/native_handler.dart:131:
libraries = libraries.where((library) => processedLibraries.add(library));
where(processedLibraries.add)

It's ok without, but I would remind the reader what `add` returns.

[ahe, 2014-06-19 13:50:10]

PTAL

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
File dart/sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
dart/sdk/lib/_internal/compiler/implementation/enqueue.dart:724: final
Set<Element> newlyEnqueuedElements = new HashSet<Element>.identity();
On 2014/06/19 09:36:19, ahe wrote:
> On 2014/06/19 09:22:32, Johnni Winther wrote:
> > Element equality is object identity so why do you need HashSet.identity ? 
> 
> Good point. I'll investigate if it matters. I doubt it will for elements.

I'm not sure I have specific evidence for this particular place, but in other
places, identity sets and maps had superior performance.

This is now encapsulated in a strategy.

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
File
dart/sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart
(right):

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
dart/sdk/lib/_internal/compiler/implementation/js_emitter/code_emitter_task.dart:1800:
// print('Invalidating ${element.enclosingClass} due to ${element}');
On 2014/06/19 09:22:32, Johnni Winther wrote:
> Remove debug line.

Done.

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
File dart/sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/340023003/diff/1/dart/sdk/lib/_internal/compi...
dart/sdk/lib/_internal/compiler/implementation/native_handler.dart:131:
libraries = libraries.where((library) => processedLibraries.add(library));
On 2014/06/19 12:00:24, floitsch wrote:
> where(processedLibraries.add)

Done.

> It's ok without, but I would remind the reader what `add` returns.

Agreed.

[Johnni Winther, 2014-06-19 14:14:04]

lgtm

https://codereview.chromium.org/340023003/diff/40001/dart/sdk/lib/_internal/c...
File dart/sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/340023003/diff/40001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/compiler.dart:509: final
CacheStrategy cacheStrategy = new CacheStrategy();
Use a noop strategy when hasIncrementalSupport is false?

[ahe, 2014-06-19 14:33:26]

Thank you!

https://codereview.chromium.org/340023003/diff/40001/dart/sdk/lib/_internal/c...
File dart/sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/340023003/diff/40001/dart/sdk/lib/_internal/c...
dart/sdk/lib/_internal/compiler/implementation/compiler.dart:509: final
CacheStrategy cacheStrategy = new CacheStrategy();
On 2014/06/19 14:14:04, Johnni Winther wrote:
> Use a noop strategy when hasIncrementalSupport is false?

Done.

I've haven't actually created a no-op class, as that seemed excessive for what I
need right now :-)

[ahe, 2014-06-19 14:36:45]

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