Port SplitIndexStore to Dart.

Port SplitIndexStore to Dart.

R=brianwilkerson@google.com, paulberry@google.com
BUG=

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

[Brian Wilkerson, 2014-06-20 14:52:14]

LGTM

[Paul Berry, 2014-06-20 15:46:02]

lgtm for now, though I hope we can address the weak reference issue sooner
rather than later (I'm worried about us forgetting to address it :))

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/index/split_store.dart (right):

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:25: Map<AnalysisContext, int>
_contextToIndex = new HashMap<AnalysisContext, int>(
The Java version of this class uses weak references to the AnalysisContexts. 
Since we don't have weak references in Dart, I think we're going to need to take
a different approach.

As written, AnalysisContexts can only be added to the ContextCodec, never
removed.  Which means that in effect, AnalysisContexts will never be garbage
collected.

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:220: }).catchError((e) {
Should probably log the stack trace as well.

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:244: }).catchError((e) {
Log the stack trace here too.

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:347: Map<RelationKeyData,
List<LocationData>> _relations = {};
Should this be a hashmap?

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:401: return new
List.from(locations);
"return locations;" should be sufficient.  There's no need to make a copy of the
list since it was freshly allocated in this function, so there are no other
references to it.

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:469: final Map<int,
List<int>> _map = new HashMap<int, List<int>>();
Would it be better to use Map<int, HashSet<int>>?  This would make add()
asymptotically faster, since it wouldn't need to scan the entire list to see if
the value was already present.  (However, it would probably be slower if most of
the sets were small).

Another advantage is that we could change the return type of get() to Set<int>,
which would more clearly express the intent.

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:664: _contextNodeRelations =
{};
1. Would a HashMap be better here?

2. Initializing this to <int, Map<int, Map<Relationship, List<LocationData>>>>{}
is safer, because it will cause the VM to do more run-time type checking (in
checked mode it will verify that every entry added to the map has the correct
type).

Similar comments apply to _contextToLibraryToUnits and _contextoUnitToLibraries.

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:703: Set<Source> _sources =
new Set();
Would HashSet be better?

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:980: return new
List.from(locations);
Should be just "return locations;"

[Brian Wilkerson, 2014-06-20 15:55:30]

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/index/split_store.dart (right):

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:25: Map<AnalysisContext, int>
_contextToIndex = new HashMap<AnalysisContext, int>(
I thought we had already added an API to notify the index when a context was
being deleted so that we could remove the context's data from the index. We
should make that same API also remove the context from the codec (and consider
removing the use of weak references on the Java side as well).

[scheglov, 2014-06-20 16:16:57]

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/index/split_store.dart (right):

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:25: Map<AnalysisContext, int>
_contextToIndex = new HashMap<AnalysisContext, int>(
You're right.
I plan to do this in the next CL.

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:220: }).catchError((e) {
On 2014/06/20 15:46:01, Paul Berry wrote:
> Should probably log the stack trace as well.

OK, I will use CaughtException.

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:244: }).catchError((e) {
On 2014/06/20 15:46:01, Paul Berry wrote:
> Log the stack trace here too.

OK, I will use CaughtException.

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:347: Map<RelationKeyData,
List<LocationData>> _relations = {};
On 2014/06/20 15:46:01, Paul Berry wrote:
> Should this be a hashmap?

Done.

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:401: return new
List.from(locations);
On 2014/06/20 15:46:01, Paul Berry wrote:
> "return locations;" should be sufficient.  There's no need to make a copy of
the
> list since it was freshly allocated in this function, so there are no other
> references to it.

Done.

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:469: final Map<int,
List<int>> _map = new HashMap<int, List<int>>();
On 2014/06/20 15:46:01, Paul Berry wrote:
> Would it be better to use Map<int, HashSet<int>>?  This would make add()
> asymptotically faster, since it wouldn't need to scan the entire list to see
if
> the value was already present.  (However, it would probably be slower if most
of
> the sets were small).
> 
> Another advantage is that we could change the return type of get() to
Set<int>,
> which would more clearly express the intent.

It is intentional.
At least in Java maps and sets are pretty expensive.
Discussed offline.

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:664: _contextNodeRelations =
{};
On 2014/06/20 15:46:01, Paul Berry wrote:
> 1. Would a HashMap be better here?
> 
> 2. Initializing this to <int, Map<int, Map<Relationship,
List<LocationData>>>>{}
> is safer, because it will cause the VM to do more run-time type checking (in
> checked mode it will verify that every entry added to the map has the correct
> type).
> 
> Similar comments apply to _contextToLibraryToUnits and
_contextoUnitToLibraries.

Done.

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:703: Set<Source> _sources =
new Set();
On 2014/06/20 15:46:01, Paul Berry wrote:
> Would HashSet be better?

Done.

https://codereview.chromium.org/348773003/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/index/split_store.dart:980: return new
List.from(locations);
On 2014/06/20 15:46:01, Paul Berry wrote:
> Should be just "return locations;"

Done.

[scheglov, 2014-06-20 16:18:35]

Committed patchset #2 manually as r37556 (presubmit successful).