Uri: use List.unmodifiable

Uri: use List.unmodifiable

BUG=
R=lrn@google.com

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

[sra1, 2015-11-04 04:34:46]

Patchset #1 (id:1) has been deleted

[sra1, 2015-11-04 04:42:31]

sra@google.com changed reviewers:
+ lrn@google.com

[sra1, 2015-11-04 04:42:32]

This form has better dart2js type inference (reasoning is better on locals than
fields) and uses List.unmodifiable to remove the UnmodifiableListView wrapper
indirection.

No performance benefit herem, but when combined with some future Array
optimizations it is about 20% faster on the 'easy' benchmark.

WDYT?

[Lasse Reichstein Nielsen, 2015-11-04 06:54:56]

LGTM with suggestions for improvement :)

https://codereview.chromium.org/1406023018/diff/20001/sdk/lib/core/uri.dart
File sdk/lib/core/uri.dart (right):

https://codereview.chromium.org/1406023018/diff/20001/sdk/lib/core/uri.dart#n...
sdk/lib/core/uri.dart:1024: var pathToSplit = !path.isEmpty &&
path.codeUnitAt(0) == _SLASH
Alternatively (I have no idea if it's better performance/code-size), you can
use:
  path.startsWith("/")
or at least use .isNotEmpty instead of !...isEmpty.

Also consider whether it's better to split the original string and remove the
first element of the resulting list instead of copying almost all of the path. 
You are doing a map below, so you can also do a "skip(1)" first in that case.

Basically:
  int skipCount = path.startsWith("/") ? 1 : 0;
...
      path.split("/").skip(skipCount).map(Uri.decodeComponent));

(It will be an efficient-length iterable, so List.unmodifiable is efficient).

https://codereview.chromium.org/1406023018/diff/20001/sdk/lib/core/uri.dart#n...
sdk/lib/core/uri.dart:1027: result = pathToSplit == ""
Parenthesize the condition (or change to .isEmpty).

(And I know it's not your code originally, but now you are here - after this
it's your name on git blame :)

[sra1, 2015-11-04 22:29:03]

https://chromiumcodereview.appspot.com/1406023018/diff/20001/sdk/lib/core/uri...
File sdk/lib/core/uri.dart (right):

https://chromiumcodereview.appspot.com/1406023018/diff/20001/sdk/lib/core/uri...
sdk/lib/core/uri.dart:1024: var pathToSplit = !path.isEmpty &&
path.codeUnitAt(0) == _SLASH
On 2015/11/04 06:54:56, Lasse Reichstein Nielsen wrote:
> Alternatively (I have no idea if it's better performance/code-size), you can
> use:
>   path.startsWith("/")
> or at least use .isNotEmpty instead of !...isEmpty.
> 
> Also consider whether it's better to split the original string and remove the
> first element of the resulting list instead of copying almost all of the path.

> You are doing a map below, so you can also do a "skip(1)" first in that case.
> 
> Basically:
>   int skipCount = path.startsWith("/") ? 1 : 0;
> ...
>       path.split("/").skip(skipCount).map(Uri.decodeComponent));
> 
> (It will be an efficient-length iterable, so List.unmodifiable is efficient).

I measured a small (5%) degradation for this pattern, mostly small because all
the work is in decodeComponent.

a.startsWith(b) does   b == a.substring(0, b.length), so the current form is
more efficient.

split returns a JS Array. SubListIterable tends to be used on a polymorphic list
implementation, making all the operations slower (See similar issue with
ListIterable https://github.com/dart-lang/sdk/issues/22671).

https://chromiumcodereview.appspot.com/1406023018/diff/20001/sdk/lib/core/uri...
sdk/lib/core/uri.dart:1027: result = pathToSplit == ""
On 2015/11/04 06:54:56, Lasse Reichstein Nielsen wrote:
> Parenthesize the condition (or change to .isEmpty).
> 
> (And I know it's not your code originally, but now you are here - after this
> it's your name on git blame :)
> 

Done.

[sra1, 2015-11-04 22:30:23]

Committed patchset #2 (id:40001) manually as
f51f25034e9dba3c5a3cf94140b63f2cdec19457 (presubmit successful).