Call special VM interpolate for single element interpolation.

Call special VM interpolate for single element interpolation.

Fixed at the intermediate language level.

R=fschneider@google.com

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

[Lasse Reichstein Nielsen, 2014-09-12 09:40:51]

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

[Lasse Reichstein Nielsen, 2014-09-12 09:40:52]

Attempt at optimizing single-element interpolation at the compiler-level.

If this works, I can probably reduce the complexity of my other CL.

[Florian Schneider, 2014-09-12 10:33:37]

LGTM.

Please check that performance improvement for the single element case is as
expected.

https://codereview.chromium.org/563973004/diff/1/runtime/lib/string_patch.dart
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/563973004/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:522: static String _interpolateSingle(final Object
o) {
final is not really necessary here.

https://codereview.chromium.org/563973004/diff/1/runtime/vm/flow_graph_builde...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/563973004/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:2392: if (length == 1) {
Drop length and replace with just arguments->length() here.

https://codereview.chromium.org/563973004/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/563973004/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.cc:2924: // This is only really useful for
numbers, so we don't bother optimizing
I dont' understand this comment because you do reach here with strings, bool or
null as well.

https://codereview.chromium.org/563973004/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/563973004/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:3810: bool IsSingleton() const { return
singleton_; }
This accessor may be removed if not needed.

https://codereview.chromium.org/563973004/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:3830: const bool singleton_;
I'd rename singleton_ to is_singleton_ for consistency with other bool members.

[Lasse Reichstein Nielsen, 2014-09-12 12:49:39]

Using the benchmark from the other CL, I get:

Before:
One-element interpolation ASCII: 3026.0 /ms
One-element interpolation non-ASCII: 1562.0 /ms
One-element interpolation Object ASCII: 3106.5 /ms
One-element interpolation Object non-ASCII: 1567.0 /ms

After:
One-element interpolation ASCII: 86358.5 /ms
One-element interpolation non-ASCII: 76202.0 /ms
One-element interpolation Object ASCII: 86492.5 /ms
One-element interpolation Object non-ASCII: 74710.5 /ms

So a ~27 x speedup on single-element interpolations.

https://codereview.chromium.org/563973004/diff/1/runtime/lib/string_patch.dart
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/563973004/diff/1/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:522: static String _interpolateSingle(final Object
o) {
On 2014/09/12 10:33:37, Florian Schneider wrote:
> final is not really necessary here.

Done.

https://codereview.chromium.org/563973004/diff/1/runtime/vm/flow_graph_builde...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/563973004/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:2392: if (length == 1) {
On 2014/09/12 10:33:37, Florian Schneider wrote:
> Drop length and replace with just arguments->length() here.

Done.

https://codereview.chromium.org/563973004/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/563973004/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.cc:2924: // This is only really useful for
numbers, so we don't bother optimizing
The point is that we don't expect to reach here very often with strings, bools
or null, The only type of such *constants* that are routinely converted to
strings are numbers. It's wasteful to use it on strings.

https://codereview.chromium.org/563973004/diff/1/runtime/vm/intermediate_lang...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/563973004/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:3810: bool IsSingleton() const { return
singleton_; }
On 2014/09/12 10:33:37, Florian Schneider wrote:
> This accessor may be removed if not needed.

Done.

https://codereview.chromium.org/563973004/diff/1/runtime/vm/intermediate_lang...
runtime/vm/intermediate_language.h:3830: const bool singleton_;
On 2014/09/12 10:33:37, Florian Schneider wrote:
> I'd rename singleton_ to is_singleton_ for consistency with other bool
members.

Done.

[Lasse Reichstein Nielsen, 2014-09-15 07:19:31]

Committed patchset #2 (id:20001) manually as 40232 (presubmit successful).

[Ivan Posva, 2014-09-15 07:47:14]

Do I read this CL correctly as:

Dart developers do write
s = "$myvalue";

instead of
s = myvalue.toString();

Except that in the second case we can optimize the toString() call based on the
dynamic type of the myvalue object at the call site. While in the string
interpolation case the type feedback gets thrown under the bus because the
toString() call in String._interpolateSingle becomes megamorphic?

Thanks,
-Ivan

[Lasse Reichstein Nielsen, 2014-09-15 08:57:13]

On 2014/09/15 07:47:14, Ivan Posva wrote:
> Do I read this CL correctly as:
> 
> Dart developers do write
> s = "$myvalue";
> 
> instead of
> s = myvalue.toString();

Yes. The former is shorter and safer (checks that the result is actually a
String).
There is no reason not to use it, except if it's implemented inefficiently, and
there is no reason to expect it to be so (or at least not to stay so).

> Except that in the second case we can optimize the toString() call based on
the
> dynamic type of the myvalue object at the call site. While in the string
> interpolation case the type feedback gets thrown under the bus because the
> toString() call in String._interpolateSingle becomes megamorphic?

Developers in general can't be expected to know the internals of the compiler.
The expression "$myvalue" is a very simple part of the core language, and I'd
not blame a developer for assuming that it is well optimized.

The current implementation goes megamorphic because it always calls a helper
function.
This CL just does away with a lot of unnecessary extra work for the one-element
case, but it doesn't avoid the megamorphism.
There is no reason why "$x" could not be optimized to be as efficient as an
inlined version of _interpolateSingle. All the information is available at the
call-site.