Avoid parsing path and query string more than once

Avoid parsing path and query string more than once

This has the side-effect of making a Uri mutable to some extend. By
not actuallt storing the passed in pathSegments and queryParameters
objects I think this is managable.

I think repeated access to uri.queryParameters should not parse the
query string into a map more than once.

R=lrn@google.com
BUG=

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

[Lasse Reichstein Nielsen, 2013-05-30 11:57:50]

lgtm

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

https://codereview.chromium.org/16108003/diff/1/sdk/lib/core/uri.dart#newcode84
sdk/lib/core/uri.dart:84: List<String> _pathSegments;
Dartdoc saying that this caches the computed return value of [pathSegments].

https://codereview.chromium.org/16108003/diff/1/sdk/lib/core/uri.dart#newcode85
sdk/lib/core/uri.dart:85: Map<String, String> _queryParameters;
Similar.

https://codereview.chromium.org/16108003/diff/1/sdk/lib/core/uri.dart#newcode175
sdk/lib/core/uri.dart:175: * be returned for each call.
"should not" isn't very safe. Could you make it unmodifiable?

https://codereview.chromium.org/16108003/diff/1/tests/corelib/uri_path_test.dart
File tests/corelib/uri_path_test.dart (right):

https://codereview.chromium.org/16108003/diff/1/tests/corelib/uri_path_test.d...
tests/corelib/uri_path_test.dart:81: Expect.isTrue(identical(uri.pathSegments,
uri.pathSegments));
I wouldn't guarantee that - it means you can't throw away your cached values if
you want to, which you may want at some point in the future.
The return value may or may not be the same object.

Or in other words: This is a performance optimization, not something we need to
check for.

https://codereview.chromium.org/16108003/diff/1/tests/corelib/uri_query_test....
File tests/corelib/uri_query_test.dart (right):

https://codereview.chromium.org/16108003/diff/1/tests/corelib/uri_query_test....
tests/corelib/uri_query_test.dart:116:
Expect.isTrue(identical(uri.queryParameters, uri.queryParameters));
Ditto here, don't specify it and don't test for it, just optimize and run!

[Søren Gjesse, 2013-05-30 13:38:25]

Added immutable list and map.

PTAL

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

https://codereview.chromium.org/16108003/diff/1/sdk/lib/core/uri.dart#newcode84
sdk/lib/core/uri.dart:84: List<String> _pathSegments;
On 2013/05/30 11:57:50, Lasse Reichstein Nielsen wrote:
> Dartdoc saying that this caches the computed return value of [pathSegments].

Done.

https://codereview.chromium.org/16108003/diff/1/sdk/lib/core/uri.dart#newcode85
sdk/lib/core/uri.dart:85: Map<String, String> _queryParameters;
On 2013/05/30 11:57:50, Lasse Reichstein Nielsen wrote:
> Similar.

Done.

https://codereview.chromium.org/16108003/diff/1/sdk/lib/core/uri.dart#newcode175
sdk/lib/core/uri.dart:175: * be returned for each call.
On 2013/05/30 11:57:50, Lasse Reichstein Nielsen wrote:
> "should not" isn't very safe. Could you make it unmodifiable?

Created an immutable list wrapper.

https://codereview.chromium.org/16108003/diff/1/tests/corelib/uri_path_test.dart
File tests/corelib/uri_path_test.dart (right):

https://codereview.chromium.org/16108003/diff/1/tests/corelib/uri_path_test.d...
tests/corelib/uri_path_test.dart:81: Expect.isTrue(identical(uri.pathSegments,
uri.pathSegments));
On 2013/05/30 11:57:50, Lasse Reichstein Nielsen wrote:
> I wouldn't guarantee that - it means you can't throw away your cached values
if
> you want to, which you may want at some point in the future.
> The return value may or may not be the same object.
> 
> Or in other words: This is a performance optimization, not something we need
to
> check for.

You are probably right. I put it in to make sure my change did what I expected.

Removed.

https://codereview.chromium.org/16108003/diff/1/tests/corelib/uri_query_test....
File tests/corelib/uri_query_test.dart (right):

https://codereview.chromium.org/16108003/diff/1/tests/corelib/uri_query_test....
tests/corelib/uri_query_test.dart:116:
Expect.isTrue(identical(uri.queryParameters, uri.queryParameters));
On 2013/05/30 11:57:50, Lasse Reichstein Nielsen wrote:
> Ditto here, don't specify it and don't test for it, just optimize and run!

Done.

[Lasse Reichstein Nielsen, 2013-06-03 07:54:27]

lgtm

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

https://codereview.chromium.org/16108003/diff/6001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:974: 
dart:collections has an UnmodifiableListView you can use for this
(it uses the internal UnmodifiableListMixin)

We use "unmodifiable" instead of "immutable", because the latter can carry a
connotation of deep immutablility, which we are not guaranteeing.

https://codereview.chromium.org/16108003/diff/6001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1023: class _ImmutableMap<K, V> implements Map<K, V> {
No UnmodifiableMapView, sorry :(
We probably should have one, but we also need Map mixins and base classes.

https://codereview.chromium.org/16108003/diff/6001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1030: void operator []=(K key, V value) => throw new
StateError("Immutable map");
UnsupportedError

[Søren Gjesse, 2013-06-03 09:17:16]

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

[Søren Gjesse, 2013-06-03 09:18:15]

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

https://codereview.chromium.org/16108003/diff/6001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:974: 
On 2013/06/03 07:54:27, Lasse Reichstein Nielsen wrote:
> dart:collections has an UnmodifiableListView you can use for this
> (it uses the internal UnmodifiableListMixin)
> 
> We use "unmodifiable" instead of "immutable", because the latter can carry a
> connotation of deep immutablility, which we are not guaranteeing.

Thanks - I missed that.

https://codereview.chromium.org/16108003/diff/6001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1023: class _ImmutableMap<K, V> implements Map<K, V> {
On 2013/06/03 07:54:27, Lasse Reichstein Nielsen wrote:
> No UnmodifiableMapView, sorry :(
> We probably should have one, but we also need Map mixins and base classes.

OK, keeping this (renaming to _UnmodifiableMap).

https://codereview.chromium.org/16108003/diff/6001/sdk/lib/core/uri.dart#newc...
sdk/lib/core/uri.dart:1030: void operator []=(K key, V value) => throw new
StateError("Immutable map");
On 2013/06/03 07:54:27, Lasse Reichstein Nielsen wrote:
> UnsupportedError

Done.