fasta: Cache relativizeUri(fileUri) on LibraryBuilder and ClassBuilder (or rather it's base class)

fasta: Cache relativizeUri(fileUri) on LibraryBuilder and ClassBuilder (or rather it's base class)

This we we don't (re)calculate the same thing for each [ProcedureBuilder] and [FieldBuilder].

R=ahe@google.com

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

[kustermann, 2017-02-21 13:50:59]

kustermann@google.com changed reviewers:
+ ahe@google.com

[ahe, 2017-02-21 14:15:23]

lgtm

https://codereview.chromium.org/2708893002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/builder/library_builder.dart (right):

https://codereview.chromium.org/2708893002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/builder/library_builder.dart:55:
LibraryBuilder._internal(this.fileUri, this.relativeFileUri)
How about just having one constructor:

LibraryBuilder(Uri fileUri)
    : fileUri = fileUri,
      relativeFileUri = relativizeUri(fileUri)
      super(null, -1, fileUri);

https://codereview.chromium.org/2708893002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/builder/type_declaration_builder.dart (right):

https://codereview.chromium.org/2708893002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/builder/type_declaration_builder.dart:35: Builder
parent, int charOffset, this.fileUri, this.relativeFileUri)
I think you can get away with one constructor here as well.

https://codereview.chromium.org/2708893002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/util/relativize.dart (left):

https://codereview.chromium.org/2708893002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/util/relativize.dart:12: String result = "$uri";
I prefer the existing style, and I know dart2js generates better code for it.

https://codereview.chromium.org/2708893002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/util/relativize.dart (right):

https://codereview.chromium.org/2708893002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/util/relativize.dart:13: final prefix =
base.toString();
String prefix = "$base";

[ahe, 2017-02-21 14:29:49]

And thank you!!!

[kustermann, 2017-02-22 14:40:16]

Description was changed from

==========
fasta: Cache relativizeUri(fileUri) on LibraryBuilder and ClassBuilder (or
rather it's base class)

This we we don't (re)calculate the same thing for each [ProcedureBuilder] and
[FieldBuilder].
==========

to

==========
fasta: Cache relativizeUri(fileUri) on LibraryBuilder and ClassBuilder (or
rather it's base class)

This we we don't (re)calculate the same thing for each [ProcedureBuilder] and
[FieldBuilder].

R=ahe@google.com

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

[kustermann, 2017-02-22 14:40:18]

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

[kustermann, 2017-02-22 14:40:28]

Thanks

https://codereview.chromium.org/2708893002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/builder/library_builder.dart (right):

https://codereview.chromium.org/2708893002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/builder/library_builder.dart:55:
LibraryBuilder._internal(this.fileUri, this.relativeFileUri)
On 2017/02/21 14:15:22, ahe wrote:
> How about just having one constructor:
> 
> LibraryBuilder(Uri fileUri)
>     : fileUri = fileUri,
>       relativeFileUri = relativizeUri(fileUri)
>       super(null, -1, fileUri);

Done

https://codereview.chromium.org/2708893002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/builder/type_declaration_builder.dart (right):

https://codereview.chromium.org/2708893002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/builder/type_declaration_builder.dart:35: Builder
parent, int charOffset, this.fileUri, this.relativeFileUri)
On 2017/02/21 14:15:22, ahe wrote:
> I think you can get away with one constructor here as well.

Done - though now I have one subexpression evaluated twice. Don't think I can
avoid that with one constructor.

https://codereview.chromium.org/2708893002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/util/relativize.dart (right):

https://codereview.chromium.org/2708893002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/util/relativize.dart:13: final prefix =
base.toString();
On 2017/02/21 14:15:22, ahe wrote:
> String prefix = "$base";

I changed it because I saw some interpolate instructions in the flow graph,
which is unnecesary work. Maybe it was just in unoptimized code.

Done.

[ahe, 2017-02-22 14:50:06]

Thank you, Martin!

https://codereview.chromium.org/2708893002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/util/relativize.dart (right):

https://codereview.chromium.org/2708893002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/util/relativize.dart:13: final prefix =
base.toString();
On 2017/02/22 14:40:28, kustermann wrote:
> On 2017/02/21 14:15:22, ahe wrote:
> > String prefix = "$base";
> 
> I changed it because I saw some interpolate instructions in the flow graph,
> which is unnecesary work. Maybe it was just in unoptimized code.
> 
> Done.

Let's not forget this. I'm pretty sure we have a benchmark that checks if string
interpolation is fast, but it may only be optimized on dart2js.