dart2js: Initial implementation of inlining.

dart2js: Initial implementation of inlining.

Perform a depth-first postorder traversal of the call graph of known
functions.  Make an inlining decision based on the vector of argument
types at the call site, and cache inlining decisions.  For positive
inlining decisions, cache the optimized function body specialized to
the receiver and argument types at the call site.

Future work:
  * Real inlining heuristics.
  * Propagate result types of inlined calls.
  * Specialized for constant arguments when it makes sense.
  * Performance improvements.

R=asgerf@google.com, sra@google.com

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

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

[Kevin Millikin (Google), 2015-12-17 13:12:26]

kmillikin@google.com changed reviewers:
+ asgerf@google.com, sra@google.com

[Kevin Millikin (Google), 2015-12-17 13:12:27]

There are nine tests whose expectations have changed.  Some of them are new
timeouts (and one new stack overflow).  The rest need to be triaged.

I will mark them with new expectations in the test status files before
committing.

[asgerf, 2015-12-17 15:40:35]

LGTM with comments addressed.

On the few benchmarks I looked at, it seems to be on par or better than the
SSA-inlining I ported over, so that's encouraging.

I am seeing some suboptimal code that I think is due to references not using
best the refinement in scope, so that's something we should look into fixing.
Running UpdateRefinements+TypePropagator at the end of inlining shows a
measurable improvement.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:2440: if (copy is Primitive) {
MutableVariable also has a hint, so we could just add hint to the interface.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:2691: Expression savedPrevious =
_current;
Do we need to cache _current?

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart (right):

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:119: if (callingConvention
!= CallingConvention.Normal) {
Could you add an exception for the new OneShotIntercepted calling convention? It
should behave like the Normal convention.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/inline.dart (right):

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:147: List<TypeMask> arguments) {
This really ought to return something instead.

I think a mixed-type return value would be suitable here, e.g:
FunctionDefinition -> inline
false -> no inline
null -> undecided

Probably false and null should be replaced by some class constants so they're
not magical.

Btw the receiver seems to have type "Typemask" with lowercase m.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:203: new CheckCpsIntegrity().check(node,
passName);
Should not check integrity in production.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:246: String formatTypeMask(TypeMask
type) {
Leftover debugging code. Btw, what's wrong with TypeMask.toString()?

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:261: String printType(nodeOrRef, String
s) {
More debugging code.

We should change the .toString methods on IR nodes and references to do
something more useful by default.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:315: /// function that takes optional
arguments not passed at the call site.
Not for this CL, but going forward maybe we should move this to the IR builder
and access it from here through the function compiler?

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:331: if (node.callingConvention !=
CallingConvention.Normal) {
Also missing a case for the new OneShotIntercepted calling convention (although
it can't occur this early in the pipeline).

Stephen suggested we make calling conventions into objects that can do some of
this work, or at least answer if it has an extra first argument.
Seems like we need this now (but also not for this CL).

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:470: _fragment.letPrim(new
NullCheck(invoke.receiver.definition,
It seems for intercepted methods we generate a null check on the interceptor?

We should use the Dart receiver instead. Maybe add the dartReceiver getter to
InvocationPrimitive:

class InvocationPrimitive {
  Primitive get dartReceiver => receiver?.definition; // Overriden by
InvokeMethod
}

The NullCheck acts as a refinement node, so it would be better with something
like,

  receiver = letPrim(new NullCheck(receiver, ..)

so the refined receiver is used in the inlined body.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:2602: bool isIntercepted =
backend.isInterceptedMethod(node.element);
Going forward I think we should change how this is done, but for now, could you
add a comment explaining what's going on?

I assume the changes below are to support the specialized argument types set by
inlining, but it wasn't obvious right away.

[sra1, 2015-12-17 23:09:14]

I see some evidence that the node receiver (i.e. interceptor), and not the dart
receiver, is being used for the receiver guard for
interceptor-calling-convention inlinees:

   Node: {
      "^": "EventTarget;ownerDocument=,parent:parentElement=,text:textContent=",
      get$nodes: function(receiver) {
        return new W._ChildNodeListLazy(receiver);
      },
      remove$0: function(receiver) {
        var v0;
        if (receiver.parentNode != null) {
          J.getInterceptor$x(v0 = receiver.parentNode).toString;
          v0.removeChild(receiver);
        }
      },

SSA:
      remove$0: function(receiver) {
        var t1 = receiver.parentNode;
        if (t1 != null)
          t1.removeChild(receiver);
      },

[sra1, 2015-12-18 02:16:26]

LGTM with receiver null check addressed.

I played with re-running type_propagation and I think the benefits are large.
1.4k smaller, but almost all of that is reducing getInterceptor call, i.e. a lot
of occurrences.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:2439: copy.type = (original as
Variable).type;
We usually use a typed local rather than 'as'.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/inline.dart (right):

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:111: }
map.putIfAbsent(element, () => <CacheEntry>())
  .add(...)

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:176: final InliningCache cache = new
InliningCache();
FYI.

SSA has 'layered' inlining caches.
The main distinction is loop vs non-loop, 'layered' meaning if there is a hit in
the non-loop cache then we should definitely inline in the loop case.

Other distinctions that are useful:

error-path vs non-error-path:
Don't inline RangeError down to the allocator - it adds a lot of code for no
benefit

execute-once vs execute-many:
SSA is very conservative inlining in lazy static initializers. On some programs
this make a big difference. On one program that had many protobufs this saved
several percent in code size.
I'm sure we could identify other used-once code, e.g. main().

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:246: String formatTypeMask(TypeMask
type) {
On 2015/12/17 15:40:34, asgerf wrote:
> Leftover debugging code. Btw, what's wrong with TypeMask.toString()?

IMHO we should change the toString() for these types to be 'terms', e.g.
ContainerTypeMask(...)

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:331: if (node.callingConvention !=
CallingConvention.Normal) {
On 2015/12/17 15:40:35, asgerf wrote:
> Also missing a case for the new OneShotIntercepted calling convention
(although
> it can't occur this early in the pipeline).
> 
> Stephen suggested we make calling conventions into objects that can do some of
> this work, or at least answer if it has an extra first argument.
> Seems like we need this now (but also not for this CL).

I think we should have a separate interceptor, (dart)receiver and
(dart)arguments, uniform for every invoke.
Static calls are just missing the first two.
We can fix that later.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:397: // AST node or targets that are
asynchronous or generator functions.
I believe 'containing try-catch' is also trivially available.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:402: invoke.receiver == null ? null :
abstractType(invoke.receiver);
Should be the dartReceiver?

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:470: _fragment.letPrim(new
NullCheck(invoke.receiver.definition,
On 2015/12/17 15:40:34, asgerf wrote:
> It seems for intercepted methods we generate a null check on the interceptor?
> 
> We should use the Dart receiver instead. Maybe add the dartReceiver getter to
> InvocationPrimitive:
> 
> class InvocationPrimitive {
>   Primitive get dartReceiver => receiver?.definition; // Overriden by
> InvokeMethod
> }
> 
> The NullCheck acts as a refinement node, so it would be better with something
> like,
> 
>   receiver = letPrim(new NullCheck(receiver, ..)
> 
> so the refined receiver is used in the inlined body.

This is what I saw in generated code.

[Kevin Millikin (Google), 2015-12-21 12:28:03]

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:2439: copy.type = (original as
Variable).type;
On 2015/12/18 02:16:26, sra1 wrote:
> We usually use a typed local rather than 'as'.

I know, but it looks horrible.  We shouldn't have to rename it, it already has a
name!

Fixed.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:2440: if (copy is Primitive) {
On 2015/12/17 15:40:34, asgerf wrote:
> MutableVariable also has a hint, so we could just add hint to the interface.

Done.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:2691: Expression savedPrevious =
_current;
On 2015/12/17 15:40:34, asgerf wrote:
> Do we need to cache _current?

Nope.  And that nicely dodges the issue of whether I call it 'current' or
'previous'.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart (right):

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:119: if (callingConvention
!= CallingConvention.Normal) {
On 2015/12/17 15:40:34, asgerf wrote:
> Could you add an exception for the new OneShotIntercepted calling convention?
It
> should behave like the Normal convention.

Done.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/inline.dart (right):

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:111: }
On 2015/12/18 02:16:26, sra1 wrote:
> map.putIfAbsent(element, () => <CacheEntry>())
>   .add(...)

Done.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:147: List<TypeMask> arguments) {
On 2015/12/17 15:40:35, asgerf wrote:
> Btw the receiver seems to have type "Typemask" with lowercase m.

So it does.  I like how that's just OK.

I've changed it to return a value and gotten rid of the state variables.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:176: final InliningCache cache = new
InliningCache();
On 2015/12/18 02:16:26, sra1 wrote:
> FYI.
> 
> SSA has 'layered' inlining caches.
> The main distinction is loop vs non-loop, 'layered' meaning if there is a hit
in
> the non-loop cache then we should definitely inline in the loop case.
> 
> Other distinctions that are useful:
> 
> error-path vs non-error-path:
> Don't inline RangeError down to the allocator - it adds a lot of code for no
> benefit
> 
> execute-once vs execute-many:
> SSA is very conservative inlining in lazy static initializers. On some
programs
> this make a big difference. On one program that had many protobufs this saved
> several percent in code size.
> I'm sure we could identify other used-once code, e.g. main().
> 
> 
> 

Acknowledged.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:203: new CheckCpsIntegrity().check(node,
passName);
On 2015/12/17 15:40:34, asgerf wrote:
> Should not check integrity in production.

Oops, that's leftover debugging code.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:246: String formatTypeMask(TypeMask
type) {
On 2015/12/17 15:40:34, asgerf wrote:
> Leftover debugging code. Btw, what's wrong with TypeMask.toString()?

Yeah, debugging code copied from another place.  TypeMask.toString() is random,
not guaranteed to work well in the S-expression representation (not that it
should, necessarily).  For example: 'null|exact=Object', where the | is
interpreted as a kind of bracket.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:246: String formatTypeMask(TypeMask
type) {
On 2015/12/18 02:16:26, sra1 wrote:
> On 2015/12/17 15:40:34, asgerf wrote:
> > Leftover debugging code. Btw, what's wrong with TypeMask.toString()?
> 
> IMHO we should change the toString() for these types to be 'terms', e.g.
> ContainerTypeMask(...)

Agreed.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:261: String printType(nodeOrRef, String
s) {
On 2015/12/17 15:40:34, asgerf wrote:
> More debugging code.
> 
> We should change the .toString methods on IR nodes and references to do
> something more useful by default.

Agreed.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:315: /// function that takes optional
arguments not passed at the call site.
On 2015/12/17 15:40:34, asgerf wrote:
> Not for this CL, but going forward maybe we should move this to the IR builder
> and access it from here through the function compiler?

Agreed.  We actually have similar code in about four other places, but they each
end up doing something a bit different.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:331: if (node.callingConvention !=
CallingConvention.Normal) {
On 2015/12/17 15:40:35, asgerf wrote:
> Also missing a case for the new OneShotIntercepted calling convention
(although
> it can't occur this early in the pipeline).
> 
> Stephen suggested we make calling conventions into objects that can do some of
> this work, or at least answer if it has an extra first argument.
> Seems like we need this now (but also not for this CL).

Acknowledged.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:331: if (node.callingConvention !=
CallingConvention.Normal) {
On 2015/12/18 02:16:26, sra1 wrote:
> On 2015/12/17 15:40:35, asgerf wrote:
> > Also missing a case for the new OneShotIntercepted calling convention
> (although
> > it can't occur this early in the pipeline).
> > 
> > Stephen suggested we make calling conventions into objects that can do some
of
> > this work, or at least answer if it has an extra first argument.
> > Seems like we need this now (but also not for this CL).
> 
> I think we should have a separate interceptor, (dart)receiver and
> (dart)arguments, uniform for every invoke.
> Static calls are just missing the first two.
> We can fix that later.

Acknowledged.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:397: // AST node or targets that are
asynchronous or generator functions.
On 2015/12/18 02:16:26, sra1 wrote:
> I believe 'containing try-catch' is also trivially available.

Yeah, I planned on investigating what to do about try/catch as future work. 
These checks are for correctness of the implementation not performance.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:402: invoke.receiver == null ? null :
abstractType(invoke.receiver);
On 2015/12/18 02:16:26, sra1 wrote:
> Should be the dartReceiver?

Yes, you are right.  That's a very dumb mistake that happened when I combined
separate code for static/instance functions.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:470: _fragment.letPrim(new
NullCheck(invoke.receiver.definition,
On 2015/12/17 15:40:34, asgerf wrote:
> It seems for intercepted methods we generate a null check on the interceptor?
> 
> We should use the Dart receiver instead. Maybe add the dartReceiver getter to
> InvocationPrimitive:
> 
> class InvocationPrimitive {
>   Primitive get dartReceiver => receiver?.definition; // Overriden by
> InvokeMethod
> }
> 
> The NullCheck acts as a refinement node, so it would be better with something
> like,
> 
>   receiver = letPrim(new NullCheck(receiver, ..)
> 
> so the refined receiver is used in the inlined body.

Done.

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1537663002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:2602: bool isIntercepted =
backend.isInterceptedMethod(node.element);
On 2015/12/17 15:40:35, asgerf wrote:
> Going forward I think we should change how this is done, but for now, could
you
> add a comment explaining what's going on?
> 
> I assume the changes below are to support the specialized argument types set
by
> inlining, but it wasn't obvious right away.

Done.

[Kevin Millikin (Google), 2015-12-22 11:50:35]

Description was changed from

==========
dart2js: Initial implementation of inlining.

Perform a depth-first postorder traversal of the call graph of known
functions.  Make an inlining decision based on the vector of argument
types at the call site, and cache inlining decisions.  For positive
inlining decisions, cache the optimized function body specialized to
the receiver and argument types at the call site.

Future work:
  * Real inlining heuristics.
  * Propagate result types of inlined calls.
  * Specialized for constant arguments when it makes sense.
  * Performance improvements.
==========

to

==========
dart2js: Initial implementation of inlining.

Perform a depth-first postorder traversal of the call graph of known
functions.  Make an inlining decision based on the vector of argument
types at the call site, and cache inlining decisions.  For positive
inlining decisions, cache the optimized function body specialized to
the receiver and argument types at the call site.

Future work:
  * Real inlining heuristics.
  * Propagate result types of inlined calls.
  * Specialized for constant arguments when it makes sense.
  * Performance improvements.

R=asgerf@google.com, sra@google.com

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

[Kevin Millikin (Google), 2015-12-22 11:50:36]

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

[Kevin Millikin (Google), 2015-12-22 16:26:31]

kmillikin@google.com changed reviewers:
+ floitsch@google.com

[Kevin Millikin (Google), 2015-12-22 16:26:31]

+floitsch

Oops, I didn't rebaseline the compiler test expectations before I committed this
initially.

I just went with rebaselining them to the current output for now.

We could disable inlining in one way or another for these tests, but they are
really end-to-end compiler pipeline tests anyway (source code in, target code
out) and so it's fragile to treat them like unit tests.

[Kevin Millikin (Google), 2015-12-22 16:27:26]

Link to the revert CL: https://codereview.chromium.org/1542003003/

[Kevin Millikin (Google), 2015-12-23 09:12:31]

Description was changed from

==========
dart2js: Initial implementation of inlining.

Perform a depth-first postorder traversal of the call graph of known
functions.  Make an inlining decision based on the vector of argument
types at the call site, and cache inlining decisions.  For positive
inlining decisions, cache the optimized function body specialized to
the receiver and argument types at the call site.

Future work:
  * Real inlining heuristics.
  * Propagate result types of inlined calls.
  * Specialized for constant arguments when it makes sense.
  * Performance improvements.

R=asgerf@google.com, sra@google.com

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

to

==========
dart2js: Initial implementation of inlining.

Perform a depth-first postorder traversal of the call graph of known
functions.  Make an inlining decision based on the vector of argument
types at the call site, and cache inlining decisions.  For positive
inlining decisions, cache the optimized function body specialized to
the receiver and argument types at the call site.

Future work:
  * Real inlining heuristics.
  * Propagate result types of inlined calls.
  * Specialized for constant arguments when it makes sense.
  * Performance improvements.

R=asgerf@google.com, sra@google.com

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

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

[Kevin Millikin (Google), 2015-12-23 09:12:33]

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