Making save a little faster:

Making save a little faster:
-removed an extraneous reconcile (happening on the UI thread)
-removed an extraneous notification to the analysis server that the file had changed
Committed: https://code.google.com/p/dart/source/detail?r=14363

[scheglov, 2012-10-29 23:06:04]

https://chromiumcodereview.appspot.com/11348017/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/CompilationUnitEditor.java
(left):

https://chromiumcodereview.appspot.com/11348017/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/CompilationUnitEditor.java:1828:
notifyAnalysisServerAboutFileChange();
We need this notification to let AS know that file changed right now, not
delayed until builder will run. Because rename refactoring will ask SearchEngine
and Indexer right after save, and will get incorrect references if we don't
notify.

[devoncarew, 2012-10-29 23:11:52]

https://chromiumcodereview.appspot.com/11348017/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/CompilationUnitEditor.java
(left):

https://chromiumcodereview.appspot.com/11348017/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/CompilationUnitEditor.java:1828:
notifyAnalysisServerAboutFileChange();
On 2012/10/29 23:06:04, scheglov wrote:
> We need this notification to let AS know that file changed right now, not
> delayed until builder will run. Because rename refactoring will ask
SearchEngine
> and Indexer right after save, and will get incorrect references if we don't
> notify.

Hmm, interesting. Do we end up doing twice as much work? Does this file get run
through the analysis server twice for each save?

[scheglov, 2012-10-29 23:28:24]

https://chromiumcodereview.appspot.com/11348017/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/CompilationUnitEditor.java
(left):

https://chromiumcodereview.appspot.com/11348017/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/CompilationUnitEditor.java:1828:
notifyAnalysisServerAboutFileChange();
On 2012/10/29 23:11:52, devoncarew wrote:
> On 2012/10/29 23:06:04, scheglov wrote:
> > We need this notification to let AS know that file changed right now, not
> > delayed until builder will run. Because rename refactoring will ask
> SearchEngine
> > and Indexer right after save, and will get incorrect references if we don't
> > notify.
> 
> Hmm, interesting. Do we end up doing twice as much work? Does this file get
run
> through the analysis server twice for each save?

You can implement something - fast, correct or easily.
Choose two of three.

Here is what we parse after saving empty library.

file:/Users/scheglov/dart/Test/Test.dart
file:/Users/scheglov/dart/Test/Test.dart
dart://core/bool.dart
dart://core/collection.dart
dart://core/comparable.dart
dart://core/date.dart
dart://core/double.dart
dart://core/duration.dart
dart://core/errors.dart
dart://core/exceptions.dart
dart://core/expando.dart
dart://core/expect.dart
dart://core/function.dart
dart://core/future.dart
dart://core/future_impl.dart
dart://core/hashable.dart
dart://core/identical.dart
dart://core/int.dart
dart://core/invocation_mirror.dart
dart://core/iterable.dart
dart://core/iterator.dart
dart://core/list.dart
dart://core/map.dart
dart://core/num.dart
dart://core/object.dart
dart://core/options.dart
dart://core/pattern.dart
dart://core/print.dart
dart://core/queue.dart
dart://core/regexp.dart
dart://core/sequences.dart
dart://core/set.dart
dart://core/sort.dart
dart://core/stopwatch.dart
dart://core/string.dart
dart://core/string_buffer.dart
dart://core/strings.dart
dart://core/type.dart
dart://core/core.dart
dart://coreimpl/hash_map_set.dart
dart://coreimpl/linked_hash_map.dart
dart://coreimpl/list.dart
dart://coreimpl/queue.dart
dart://coreimpl/regexp.dart
dart://coreimpl/string.dart
dart://coreimpl/coreimpl.dart
dart://collection/arrays.dart
dart://collection/collections.dart
dart://collection/maps.dart
dart://collection/splay_tree.dart
dart://collection/collection.dart
file:/Users/scheglov/dart/Test/Test.dart
file:/Users/scheglov/dart/Test/Test.dart
file:/Users/scheglov/dart/Test/Test.dart
dart://core/bool.dart
dart://core/collection.dart
dart://core/comparable.dart
dart://core/date.dart
dart://core/double.dart
dart://core/duration.dart
dart://core/errors.dart
dart://core/exceptions.dart
dart://core/expando.dart
dart://core/expect.dart
dart://core/function.dart
dart://core/future.dart
dart://core/future_impl.dart
dart://core/hashable.dart
dart://core/identical.dart
dart://core/int.dart
dart://core/invocation_mirror.dart
dart://core/iterable.dart
dart://core/iterator.dart
dart://core/list.dart
dart://core/map.dart
dart://core/num.dart
dart://core/object.dart
dart://core/options.dart
dart://core/pattern.dart
dart://core/print.dart
dart://core/queue.dart
dart://core/regexp.dart
dart://core/sequences.dart
dart://core/set.dart
dart://core/sort.dart
dart://core/stopwatch.dart
dart://core/string.dart
dart://core/string_buffer.dart
dart://core/strings.dart
dart://core/type.dart
dart://core/core.dart
dart://coreimpl/hash_map_set.dart
dart://coreimpl/linked_hash_map.dart
dart://coreimpl/list.dart
dart://coreimpl/queue.dart
dart://coreimpl/regexp.dart
dart://coreimpl/string.dart
dart://coreimpl/coreimpl.dart
dart://collection/arrays.dart
dart://collection/collections.dart
dart://collection/maps.dart
dart://collection/splay_tree.dart
dart://collection/collection.dart

[pquitslund, 2012-10-30 03:57:45]

https://chromiumcodereview.appspot.com/11348017/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/CompilationUnitEditor.java
(left):

https://chromiumcodereview.appspot.com/11348017/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/CompilationUnitEditor.java:1828:
notifyAnalysisServerAboutFileChange();
On 2012/10/29 23:06:04, scheglov wrote:
> We need this notification to let AS know that file changed right now, not
> delayed until builder will run. Because rename refactoring will ask
SearchEngine
> and Indexer right after save, and will get incorrect references if we don't
> notify.

Is there any reason the refactoring engine can't initiate the reconcile?  It
seems a bit heavy to tax every save in case it will be followed by a
refactoring.

[scheglov, 2012-10-30 04:20:29]

lgtm

https://chromiumcodereview.appspot.com/11348017/diff/1/editor/tools/plugins/c...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/CompilationUnitEditor.java
(left):

https://chromiumcodereview.appspot.com/11348017/diff/1/editor/tools/plugins/c...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/CompilationUnitEditor.java:1828:
notifyAnalysisServerAboutFileChange();
On 2012/10/30 03:57:45, pquitslund wrote:
> On 2012/10/29 23:06:04, scheglov wrote:
> > We need this notification to let AS know that file changed right now, not
> > delayed until builder will run. Because rename refactoring will ask
> SearchEngine
> > and Indexer right after save, and will get incorrect references if we don't
> > notify.
> 
> Is there any reason the refactoring engine can't initiate the reconcile?  It
> seems a bit heavy to tax every save in case it will be followed by a
> refactoring.

Good idea.
Only refactoring is so time sensitive, so it is enough to notify in refactoring
save helper.

https://codereview.chromium.org/11301033

[devoncarew, 2012-10-31 16:41:49]

Adding a few more reviewers; the scope of this CL has creeped up.

A straight-line save is now 60% faster; the semantic highlighter is now 70%+
faster. I've identified a few more areas where we can make performance gains but
those are for future CLs.

[scheglov, 2012-10-31 16:57:25]

https://chromiumcodereview.appspot.com/11348017/diff/8001/compiler/java/com/g...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):

https://chromiumcodereview.appspot.com/11348017/diff/8001/compiler/java/com/g...
compiler/java/com/google/dart/compiler/parser/DartParser.java:1739: for (int i =
0, size = parameters.size(); i < size; i++) {
Why?!
We can not know instance of which type it is, and some implementations of List
have very slow access by index.

[devoncarew, 2012-10-31 17:51:35]

https://chromiumcodereview.appspot.com/11348017/diff/8001/compiler/java/com/g...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):

https://chromiumcodereview.appspot.com/11348017/diff/8001/compiler/java/com/g...
compiler/java/com/google/dart/compiler/parser/DartParser.java:1739: for (int i =
0, size = parameters.size(); i < size; i++) {
On 2012/10/31 16:57:25, scheglov wrote:
> Why?!
> We can not know instance of which type it is, and some implementations of List
> have very slow access by index.

It's always an ArrayList or a NodeList, which is backed by an ArrayList. get(i)
for ArrayList is O(1), so this will have fast indexing behavior.

The syntax sugar of the for (element: list) structure creates a new
list.iterator(), and calls hasNext and next for each iteration. We got a good
win out of the profiler when making these changes. Places where I did this would
often drop off the list of hotspots.

Making NodeList itself faster would be a good arc of work. It would change from
delegating to an ArrayList to implementing the List methods itself. Our list
manipulation (get(), size(), creating, iterating, and copying) can be as much as
20% or more of our parse time.

[scheglov, 2012-10-31 18:02:53]

LGTM

https://chromiumcodereview.appspot.com/11348017/diff/8001/compiler/java/com/g...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):

https://chromiumcodereview.appspot.com/11348017/diff/8001/compiler/java/com/g...
compiler/java/com/google/dart/compiler/parser/DartParser.java:1739: for (int i =
0, size = parameters.size(); i < size; i++) {
On 2012/10/31 17:51:36, devoncarew wrote:
> On 2012/10/31 16:57:25, scheglov wrote:
> > Why?!
> > We can not know instance of which type it is, and some implementations of
List
> > have very slow access by index.
> 
> It's always an ArrayList or a NodeList, which is backed by an ArrayList.
get(i)
> for ArrayList is O(1), so this will have fast indexing behavior.
> 
> The syntax sugar of the for (element: list) structure creates a new
> list.iterator(), and calls hasNext and next for each iteration. We got a good
> win out of the profiler when making these changes. Places where I did this
would
> often drop off the list of hotspots.
> 
> Making NodeList itself faster would be a good arc of work. It would change
from
> delegating to an ArrayList to implementing the List methods itself. Our list
> manipulation (get(), size(), creating, iterating, and copying) can be as much
as
> 20% or more of our parse time.

OK, agree.

[messick, 2012-10-31 18:13:26]

lgtm

https://chromiumcodereview.appspot.com/11348017/diff/8001/editor/tools/plugin...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/model/DartElementImpl.java
(right):

https://chromiumcodereview.appspot.com/11348017/diff/8001/editor/tools/plugin...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/model/DartElementImpl.java:461:
return getElementName().hashCode() * 17 + parent.hashCode();
Have you checked hash code distribution? If there are a lot of collisions that
will cause slowness that is harder to measure.

https://chromiumcodereview.appspot.com/11348017/diff/8001/editor/tools/plugin...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/SemanticHighlightingReconciler.java
(right):

https://chromiumcodereview.appspot.com/11348017/diff/8001/editor/tools/plugin...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/SemanticHighlightingReconciler.java:120:
private final void addPosition(int offset, int length, Highlighting
highlighting) {
I assume adding final is an attempt to get this method inlined. Can you measure
a difference in overall performance? If so, I suggest adding a comment (with
numbers) so that future programmers do not delete 'final'.

[pquitslund, 2012-10-31 18:16:55]

lgtm

[devoncarew, 2012-10-31 18:22:22]

https://chromiumcodereview.appspot.com/11348017/diff/8001/editor/tools/plugin...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/model/DartElementImpl.java
(right):

https://chromiumcodereview.appspot.com/11348017/diff/8001/editor/tools/plugin...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/model/DartElementImpl.java:461:
return getElementName().hashCode() * 17 + parent.hashCode();
On 2012/10/31 18:13:26, messick wrote:
> Have you checked hash code distribution? If there are a lot of collisions that
> will cause slowness that is harder to measure.

I have not checked the hash code distribution, but that is a good point. I think
a good arc of work would be to make this hashCode impl faster (and to verify or
improve the quality of its hash). getElementName() is called a lot, and mostly
from this method. This method itself is called a large number of times when
checking for the presence of elements in maps. That's done in the model, when
the delta processor is busy updating it based on resource changes.

https://chromiumcodereview.appspot.com/11348017/diff/8001/editor/tools/plugin...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/SemanticHighlightingReconciler.java
(right):

https://chromiumcodereview.appspot.com/11348017/diff/8001/editor/tools/plugin...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/SemanticHighlightingReconciler.java:120:
private final void addPosition(int offset, int length, Highlighting
highlighting) {
On 2012/10/31 18:13:26, messick wrote:
> I assume adding final is an attempt to get this method inlined. Can you
measure
> a difference in overall performance? If so, I suggest adding a comment (with
> numbers) so that future programmers do not delete 'final'.

I'll add those comments here and in other places where I put in finals.

I added the final here in my first pass through, when I saw how often much work
this method was doing. The method was a lot smaller then however. After the
algorithmic rewrite, I may remove the final. I doubt any compiler will inline a
method of this size...

[danrubel, 2012-10-31 18:40:50]

Per our conversation, LGTM