Use FastUri instead of Uri in analyzer.

Use FastUri instead of Uri in analyzer.

This makes DDC compilation about 25% faster - from 1550 to 1200 ms.

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

Committed: https://github.com/dart-lang/sdk/commit/d78cf637b103e009e5030abd5704b85399cd329d

[Paul Berry, 2016-05-20 16:18:12]

https://codereview.chromium.org/2003633002/diff/1/pkg/analyzer/lib/src/util/f...
File pkg/analyzer/lib/src/util/fast_uri.dart (right):

https://codereview.chromium.org/2003633002/diff/1/pkg/analyzer/lib/src/util/f...
pkg/analyzer/lib/src/util/fast_uri.dart:18: final List<int> _pathSegmentStarts;
A doc comment would be helpful here to explain the following subtleties:

- Starts are character indices into [_text] (not [_path], as one might guess).

- The indices in this list point to '/'s, not to the path components themselves
(i.e. for 'foo/bar/baz', the starts are [3, 7], not [4, 8] as I originally
guessed).  Perhaps a better name for this variable would be `_slashIndices` or
something.

- If the path contains two slashes in a row, both slashes are included (even
though a leading '//' is dropped from _path--not sure if this was intentional or
not).

- If the path ends in '/', the final '/' is considered a path segment start.

However, having said all that, it looks like the only element of
[_pathSegmentStarts] that is actually used is the last element.  Consider
changing to an int called `_lastSlashIndex`.

https://codereview.chromium.org/2003633002/diff/1/pkg/analyzer/lib/src/util/f...
pkg/analyzer/lib/src/util/fast_uri.dart:25: Uri _cachedFallbackUri;
Seeing the word "fallback" caused me to make an incorrect assumption about this
code: I assumed that it implemented the full functionality of [Uri]; it simply
had a faster implementation of the common use cases.  However, now I see that in
fact the purpose of this field is to allow us to make use of [Uri] for the
methods that are more complex and less in need of a fast implementation.

A doc comment would really have helped make this clearer.  Perhaps something
like "Full [Uri] object computed on demand; we fall back to this for some of the
more complex methods of [Uri] that are less in need of a fast implementation."

https://codereview.chromium.org/2003633002/diff/1/pkg/analyzer/lib/src/util/f...
pkg/analyzer/lib/src/util/fast_uri.dart:164: String newText = _text.substring(0,
_pathSegmentStarts.last + 1) + refPath;
Bug: this will throw an exception if the URI does not contain any '/'
characters, because [_pathSegmentStarts] will be empty.

https://codereview.chromium.org/2003633002/diff/1/pkg/analyzer/lib/src/util/f...
pkg/analyzer/lib/src/util/fast_uri.dart:176: static Uri parse(String text) {
A doc comment would be helpful here too.  In particular, I don't know whether
this method supports the full URI syntax, or just a subset of it.  (I believe it
supports the full URI syntax because of the call to Uri.parse, but that's not
obvious without reading the implementation, and based on comments alone I would
have assumed the opposite).

Also the doc comment should mention that the function is internally memoized, so
that clients aren't tempted to perform redundant memoization.

https://codereview.chromium.org/2003633002/diff/1/pkg/analyzer/lib/src/util/f...
pkg/analyzer/lib/src/util/fast_uri.dart:199: static _UriData _parse(String text)
{
A doc comment would be nice here as well, since there are some subtle behaviors.
 In particular, the comment should explain that if `text` uses URI features that
aren't supported by FastUri, `null` is returned.

https://codereview.chromium.org/2003633002/diff/1/pkg/analyzer/lib/src/util/f...
pkg/analyzer/lib/src/util/fast_uri.dart:228: return new _UriData(scheme, path,
pathSegmentStarts);
It seems awkward to have to create this intermediate data structure when it will
be immediately consumed (and discarded) by the caller.

How about instead changing the return type of _parse to `Uri`, and replace this
line with `return new FastUri(text, scheme, path, pathSegmentStarts);`.  Then
the intermediate data structure can go away completely, and the `parse` function
above becomes a one-liner:

static Uri parse(String text) => _cache[text] ??= (_parse(text) ??
Uri.parse(text));

https://codereview.chromium.org/2003633002/diff/20001/pkg/analyzer/test/src/u...
File pkg/analyzer/test/src/util/fast_uri_test.dart (right):

https://codereview.chromium.org/2003633002/diff/20001/pkg/analyzer/test/src/u...
pkg/analyzer/test/src/util/fast_uri_test.dart:19: class _FastUriTest {
Include test cases for:

- URI ending in '/'
- URI containing no '/' characters

[scheglov, 2016-05-20 16:47:23]

PTAL

https://codereview.chromium.org/2003633002/diff/1/pkg/analyzer/lib/src/util/f...
File pkg/analyzer/lib/src/util/fast_uri.dart (right):

https://codereview.chromium.org/2003633002/diff/1/pkg/analyzer/lib/src/util/f...
pkg/analyzer/lib/src/util/fast_uri.dart:18: final List<int> _pathSegmentStarts;
On 2016/05/20 16:18:11, Paul Berry wrote:
> A doc comment would be helpful here to explain the following subtleties:
> 
> - Starts are character indices into [_text] (not [_path], as one might guess).
> 
> - The indices in this list point to '/'s, not to the path components
themselves
> (i.e. for 'foo/bar/baz', the starts are [3, 7], not [4, 8] as I originally
> guessed).  Perhaps a better name for this variable would be `_slashIndices` or
> something.
> 
> - If the path contains two slashes in a row, both slashes are included (even
> though a leading '//' is dropped from _path--not sure if this was intentional
or
> not).
> 
> - If the path ends in '/', the final '/' is considered a path segment start.
> 
> However, having said all that, it looks like the only element of
> [_pathSegmentStarts] that is actually used is the last element.  Consider
> changing to an int called `_lastSlashIndex`.

Done.

https://codereview.chromium.org/2003633002/diff/1/pkg/analyzer/lib/src/util/f...
pkg/analyzer/lib/src/util/fast_uri.dart:25: Uri _cachedFallbackUri;
On 2016/05/20 16:18:11, Paul Berry wrote:
> Seeing the word "fallback" caused me to make an incorrect assumption about
this
> code: I assumed that it implemented the full functionality of [Uri]; it simply
> had a faster implementation of the common use cases.  However, now I see that
in
> fact the purpose of this field is to allow us to make use of [Uri] for the
> methods that are more complex and less in need of a fast implementation.
> 
> A doc comment would really have helped make this clearer.  Perhaps something
> like "Full [Uri] object computed on demand; we fall back to this for some of
the
> more complex methods of [Uri] that are less in need of a fast implementation."

Done.

https://codereview.chromium.org/2003633002/diff/1/pkg/analyzer/lib/src/util/f...
pkg/analyzer/lib/src/util/fast_uri.dart:164: String newText = _text.substring(0,
_pathSegmentStarts.last + 1) + refPath;
On 2016/05/20 16:18:11, Paul Berry wrote:
> Bug: this will throw an exception if the URI does not contain any '/'
> characters, because [_pathSegmentStarts] will be empty.

Fixed.

https://codereview.chromium.org/2003633002/diff/1/pkg/analyzer/lib/src/util/f...
pkg/analyzer/lib/src/util/fast_uri.dart:176: static Uri parse(String text) {
On 2016/05/20 16:18:11, Paul Berry wrote:
> A doc comment would be helpful here too.  In particular, I don't know whether
> this method supports the full URI syntax, or just a subset of it.  (I believe
it
> supports the full URI syntax because of the call to Uri.parse, but that's not
> obvious without reading the implementation, and based on comments alone I
would
> have assumed the opposite).
> 
> Also the doc comment should mention that the function is internally memoized,
so
> that clients aren't tempted to perform redundant memoization.

Done.

https://codereview.chromium.org/2003633002/diff/1/pkg/analyzer/lib/src/util/f...
pkg/analyzer/lib/src/util/fast_uri.dart:199: static _UriData _parse(String text)
{
On 2016/05/20 16:18:11, Paul Berry wrote:
> A doc comment would be nice here as well, since there are some subtle
behaviors.
>  In particular, the comment should explain that if `text` uses URI features
that
> aren't supported by FastUri, `null` is returned.

Done.

https://codereview.chromium.org/2003633002/diff/1/pkg/analyzer/lib/src/util/f...
pkg/analyzer/lib/src/util/fast_uri.dart:228: return new _UriData(scheme, path,
pathSegmentStarts);
On 2016/05/20 16:18:11, Paul Berry wrote:
> It seems awkward to have to create this intermediate data structure when it
will
> be immediately consumed (and discarded) by the caller.
> 
> How about instead changing the return type of _parse to `Uri`, and replace
this
> line with `return new FastUri(text, scheme, path, pathSegmentStarts);`.  Then
> the intermediate data structure can go away completely, and the `parse`
function
> above becomes a one-liner:
> 
> static Uri parse(String text) => _cache[text] ??= (_parse(text) ??
> Uri.parse(text));

Done.

https://codereview.chromium.org/2003633002/diff/20001/pkg/analyzer/test/src/u...
File pkg/analyzer/test/src/util/fast_uri_test.dart (right):

https://codereview.chromium.org/2003633002/diff/20001/pkg/analyzer/test/src/u...
pkg/analyzer/test/src/util/fast_uri_test.dart:19: class _FastUriTest {
On 2016/05/20 16:18:11, Paul Berry wrote:
> Include test cases for:
> 
> - URI ending in '/'
> - URI containing no '/' characters

Done.

[Paul Berry, 2016-05-20 16:50:39]

lgtm

https://codereview.chromium.org/2003633002/diff/40001/pkg/analyzer/lib/src/ut...
File pkg/analyzer/lib/src/util/fast_uri.dart (right):

https://codereview.chromium.org/2003633002/diff/40001/pkg/analyzer/lib/src/ut...
pkg/analyzer/lib/src/util/fast_uri.dart:196: * This method also performs
memoization, so that usually the same instance
Why "usually"?  From the implementation it looks like the same instance is
always returned for the same text.

[Brian Wilkerson, 2016-05-20 16:56:54]

lgtm

https://codereview.chromium.org/2003633002/diff/40001/pkg/analyzer/lib/src/ut...
File pkg/analyzer/lib/src/util/fast_uri.dart (right):

https://codereview.chromium.org/2003633002/diff/40001/pkg/analyzer/lib/src/ut...
pkg/analyzer/lib/src/util/fast_uri.dart:113: if (other is FastUri) {
Won't this be the more common case? If so, it might be worthwhile to test for
this first:

if (other is FastUri) {
  return _text == other._text;
} else if (other is Uri) {
  return _fallbackUri == other;
}
return false;

[scheglov, 2016-05-20 17:33:24]

Description was changed from

==========
Use FastUri instead of Uri in analyzer.

This makes DDC compilation about 25% faster - from 1550 to 1200 ms.

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

to

==========
Use FastUri instead of Uri in analyzer.

This makes DDC compilation about 25% faster - from 1550 to 1200 ms.

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

Committed:
https://github.com/dart-lang/sdk/commit/d78cf637b103e009e5030abd5704b85399cd329d
==========

[scheglov, 2016-05-20 17:33:25]

Committed patchset #3 (id:40001) manually as
d78cf637b103e009e5030abd5704b85399cd329d (presubmit successful).