First version of the new generators implementation.

First version of the new generators implementation.

Behind --ignition-generators. Does not yet support Turbofan.

TBR=bmeurer@chromium.org
BUG=v8:4907
LOG=n

Committed: https://crrev.com/d0ccddd0324aa9391e76b611481bdc47f0f073de
Cr-Commit-Position: refs/heads/master@{#35584}

[neis, 2016-04-14 13:35:13]

Description was changed from

==========
First version of the new generators implementation.

Does not yet support Turbofan.

BUG=v8:4907
LOG=n
==========

to

==========
First version of the new generators implementation.

Behind --ignition-generators. Does not yet support Turbofan.

BUG=v8:4907
LOG=n
==========

[neis, 2016-04-14 13:35:26]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-14 13:35:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1884183002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1884183002/1

[commit-bot: I haz the power, 2016-04-14 14:01:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-14 14:01:01]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/150)
  v8_linux_arm_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...)

[neis, 2016-04-14 15:17:17]

Patchset #1 (id:1) has been deleted

[neis, 2016-04-14 15:17:29]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-14 15:17:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1884183002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1884183002/20001

[commit-bot: I haz the power, 2016-04-14 15:48:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-14 15:48:39]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/163)
  v8_linux_arm_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...)

[neis, 2016-04-15 07:39:12]

neis@chromium.org changed reviewers:
+ jarin@chromium.org, rmcilroy@chromium.org

[neis, 2016-04-15 07:40:07]

PTAL. Will port to the remaining platforms once everything looks good.

[rmcilroy, 2016-04-15 13:17:47]

Very nice :).

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:644: resume_points_.clear();
Do we need the clear? it should be empty when we reach here anyway.

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2273: int id = yield_count_++;
Rather than doing this id naming implicitly, would it be safer to have the Yield
AST object include an ID (e.g., the one used for state calculation) which we use
as the id here and as the index to the resume_points_ ?

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2276: Register value =
VisitForRegisterValue(expr->expression());
nit - maybe scope the pre-yield, and post-resume parts with seperate inner
scopes, and have each allocate it's own RegisterAllocationScope, that way you
will free up temporary registers between the yield and resume points.

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2301:
->CallRuntime(Runtime::kGeneratorGetResumeMode, generator, 1)
Maybe you could have a runtime function which returns both Input and Resume mode
rather than making two successive runtime calls here?

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2317: .Jump(&resume_with_next);
(micro optimization) - presumably JSGeneratorObject::kNext is the most common
resume mode, if we test for that first we would execute less bytecode in the
common case.

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2348: // are converted from assignment
statements in Rewriter::ReWrite pass. An
Thanks!

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:112: void BuildGeneratorPrologue();
nit - We've been a bit inconsistent with Visit vs. Build, but I think
VisitGeneratorPrologue is probably more appropriate here.

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:209: ZoneVector<BytecodeLabel>
resume_points_;  // For generator functions.
just generator_resume_points_, then you don't need to comment?

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:212: int yield_count_;  // For generator
functions: number of yields seen so far.
ditto?

https://codereview.chromium.org/1884183002/diff/20001/src/runtime/runtime-gen...
File src/runtime/runtime-generator.cc (right):

https://codereview.chromium.org/1884183002/diff/20001/src/runtime/runtime-gen...
src/runtime/runtime-generator.cc:172: generator->set_continuation(state->value()
+ 1);  // continuation 0 is special
Is continuation '0' only special in the FCG generator implementation, or is it
special for the Ignition version too? If the former, could you add a TODO to
avoid this offsetting by 1 once the FCG implementation disappears.

https://codereview.chromium.org/1884183002/diff/20001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1884183002/diff/20001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:552: rax, rax,
SharedFunctionInfo::kFormalParameterCountOffset);
I don't think we need the formal parameter count in rax here, but I may be
missing something.

https://codereview.chromium.org/1884183002/diff/20001/test/mjsunit/ignition/g...
File test/mjsunit/ignition/generators.js (right):

https://codereview.chromium.org/1884183002/diff/20001/test/mjsunit/ignition/g...
test/mjsunit/ignition/generators.js:7: 
Great tests. Could you also add a couple of simple tests to test-interpreter and
test-bytecode-generator as well please?

[Michael Starzinger, 2016-04-15 14:13:47]

I just looked at the plumbing around the interpreter, that is looking good.
Didn't look inside the interpreter directory at all.

https://codereview.chromium.org/1884183002/diff/20001/src/runtime/runtime-gen...
File src/runtime/runtime-generator.cc (right):

https://codereview.chromium.org/1884183002/diff/20001/src/runtime/runtime-gen...
src/runtime/runtime-generator.cc:159: CHECK(frame->type() ==
StackFrame::INTERPRETED);
nit: CHECK_EQ

https://codereview.chromium.org/1884183002/diff/20001/src/runtime/runtime-gen...
src/runtime/runtime-generator.cc:187: CHECK(frame->type() ==
StackFrame::INTERPRETED);
nit: CHECK_EQ

https://codereview.chromium.org/1884183002/diff/20001/test/mjsunit/ignition/g...
File test/mjsunit/ignition/generators.js (right):

https://codereview.chromium.org/1884183002/diff/20001/test/mjsunit/ignition/g...
test/mjsunit/ignition/generators.js:302: // The rest is copied 1:1 from
harmony/generators.js.
suggestion: Instead of duplicating these tests here, can we just add the
--ignition-generators flag to harmony/generators.js? Because the variants will
make sure that we still run it with FCG as well anyways. So the only combination
we would not cover would be --ignition but --no-ignition-generators, IMHO not
covering that all too thoroughly for a while is fine. WDYT?

[neis, 2016-04-18 08:04:16]

neis@chromium.org changed reviewers:
+ bmeurer@chromium.org

[neis, 2016-04-18 08:04:18]

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:644: resume_points_.clear();
On 2016/04/15 13:17:46, rmcilroy wrote:
> Do we need the clear? it should be empty when we reach here anyway.

MakeBytecode could be called several times on the same BytecodeGenerator
instance, right?

If that's not supposed to happen, then I don't understand why CompilationInfo is
passed to MakeBytecode and not to the BytecodeGenerator constructor.

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2273: int id = yield_count_++;
On 2016/04/15 13:17:46, rmcilroy wrote:
> Rather than doing this id naming implicitly, would it be safer to have the
Yield
> AST object include an ID (e.g., the one used for state calculation) which we
use
> as the id here and as the index to the resume_points_ ?

I don't think that would be safer and I don't see any advantage. Also, my
transformation for dispatching in loops will rely on yields being numbered
consecutively.

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2276: Register value =
VisitForRegisterValue(expr->expression());
On 2016/04/15 13:17:46, rmcilroy wrote:
> nit - maybe scope the pre-yield, and post-resume parts with seperate inner
> scopes, and have each allocate it's own RegisterAllocationScope, that way you
> will free up temporary registers between the yield and resume points.

Thanks, I didn't think of that.  I added a scope for the post-resume part.  One
of the registers from the pre-resume part is used later on.

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2301:
->CallRuntime(Runtime::kGeneratorGetResumeMode, generator, 1)
On 2016/04/15 13:17:46, rmcilroy wrote:
> Maybe you could have a runtime function which returns both Input and Resume
mode
> rather than making two successive runtime calls here?

Let's keep it as is for now.  I think we want to get rid of these calls in the
long run anyway.

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2317: .Jump(&resume_with_next);
On 2016/04/15 13:17:46, rmcilroy wrote:
> (micro optimization) - presumably JSGeneratorObject::kNext is the most common
> resume mode, if we test for that first we would execute less bytecode in the
> common case.

Done.

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.h (right):

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:112: void BuildGeneratorPrologue();
On 2016/04/15 13:17:46, rmcilroy wrote:
> nit - We've been a bit inconsistent with Visit vs. Build, but I think
> VisitGeneratorPrologue is probably more appropriate here.

Done.

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:209: ZoneVector<BytecodeLabel>
resume_points_;  // For generator functions.
On 2016/04/15 13:17:46, rmcilroy wrote:
> just generator_resume_points_, then you don't need to comment?

Done.

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.h:212: int yield_count_;  // For generator
functions: number of yields seen so far.
On 2016/04/15 13:17:46, rmcilroy wrote:
> ditto?

Done.

https://codereview.chromium.org/1884183002/diff/20001/src/runtime/runtime-gen...
File src/runtime/runtime-generator.cc (right):

https://codereview.chromium.org/1884183002/diff/20001/src/runtime/runtime-gen...
src/runtime/runtime-generator.cc:159: CHECK(frame->type() ==
StackFrame::INTERPRETED);
On 2016/04/15 14:13:47, Michael Starzinger wrote:
> nit: CHECK_EQ

Done.

https://codereview.chromium.org/1884183002/diff/20001/src/runtime/runtime-gen...
src/runtime/runtime-generator.cc:172: generator->set_continuation(state->value()
+ 1);  // continuation 0 is special
On 2016/04/15 13:17:46, rmcilroy wrote:
> Is continuation '0' only special in the FCG generator implementation, or is it
> special for the Ignition version too? If the former, could you add a TODO to
> avoid this offsetting by 1 once the FCG implementation disappears.

Both actually, because of some shared code.  But I changed that code now so that
0 is no longer special, and got rid of the +1/-1 here.

https://codereview.chromium.org/1884183002/diff/20001/src/runtime/runtime-gen...
src/runtime/runtime-generator.cc:187: CHECK(frame->type() ==
StackFrame::INTERPRETED);
On 2016/04/15 14:13:47, Michael Starzinger wrote:
> nit: CHECK_EQ

Done.

https://codereview.chromium.org/1884183002/diff/20001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1884183002/diff/20001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:552: rax, rax,
SharedFunctionInfo::kFormalParameterCountOffset);
On 2016/04/15 13:17:47, rmcilroy wrote:
> I don't think we need the formal parameter count in rax here, but I may be
> missing something.

Benedikt suggested to keep it this way for consistency.

https://codereview.chromium.org/1884183002/diff/20001/test/mjsunit/ignition/g...
File test/mjsunit/ignition/generators.js (right):

https://codereview.chromium.org/1884183002/diff/20001/test/mjsunit/ignition/g...
test/mjsunit/ignition/generators.js:7: 
On 2016/04/15 13:17:47, rmcilroy wrote:
> Great tests. Could you also add a couple of simple tests to test-interpreter
and
> test-bytecode-generator as well please?

Acknowledged.

https://codereview.chromium.org/1884183002/diff/20001/test/mjsunit/ignition/g...
test/mjsunit/ignition/generators.js:302: // The rest is copied 1:1 from
harmony/generators.js.
On 2016/04/15 14:13:47, Michael Starzinger wrote:
> suggestion: Instead of duplicating these tests here, can we just add the
> --ignition-generators flag to harmony/generators.js? Because the variants will
> make sure that we still run it with FCG as well anyways. So the only
combination
> we would not cover would be --ignition but --no-ignition-generators, IMHO not
> covering that all too thoroughly for a while is fine. WDYT?

Fine with me.  I removed the implication between --ignition-generators and
--ignition and merged the two test files.

[neis, 2016-04-18 10:01:07]

Added some basic cctests now.

[rmcilroy, 2016-04-18 10:01:57]

LGTM once ports are done and test-bytecode-generator tests are added.

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:644: resume_points_.clear();
On 2016/04/18 08:04:17, neis wrote:
> On 2016/04/15 13:17:46, rmcilroy wrote:
> > Do we need the clear? it should be empty when we reach here anyway.
> 
> MakeBytecode could be called several times on the same BytecodeGenerator
> instance, right?
> 
> If that's not supposed to happen, then I don't understand why CompilationInfo
is
> passed to MakeBytecode and not to the BytecodeGenerator constructor.

No it's only ever used for a single compile info. You are right though, we
should probably change the constructor to take the CompileInfo, I'll upload a CL
to make that change.

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2273: int id = yield_count_++;
On 2016/04/18 08:04:17, neis wrote:
> On 2016/04/15 13:17:46, rmcilroy wrote:
> > Rather than doing this id naming implicitly, would it be safer to have the
> Yield
> > AST object include an ID (e.g., the one used for state calculation) which we
> use
> > as the id here and as the index to the resume_points_ ?
> 
> I don't think that would be safer and I don't see any advantage. Also, my
> transformation for dispatching in loops will rely on yields being numbered
> consecutively.

OK. I'm fine with this for this CL, but maybe it's worth considering maintaining
yield_count_ and resume_points_ in a small class to abstract the yield / resume
point maintanance (particularly if the dispatching in loops CL adds more logic
to this).

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2301:
->CallRuntime(Runtime::kGeneratorGetResumeMode, generator, 1)
On 2016/04/18 08:04:17, neis wrote:
> On 2016/04/15 13:17:46, rmcilroy wrote:
> > Maybe you could have a runtime function which returns both Input and Resume
> mode
> > rather than making two successive runtime calls here?
> 
> Let's keep it as is for now.  I think we want to get rid of these calls in the
> long run anyway.

Acknowledged.

[rmcilroy, 2016-04-18 10:04:33]

That was quick on the tests :). Tests and generate-bytecode-expectations.cc
changes LGTM too, thanks!

[neis, 2016-04-18 13:22:55]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-18 13:23:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1884183002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1884183002/120001

[commit-bot: I haz the power, 2016-04-18 13:48:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-18 13:48:03]

Dry run: This issue passed the CQ dry run.

[Jarin, 2016-04-18 13:55:06]

lgtm. Very cool.

https://codereview.chromium.org/1884183002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1884183002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:2320: .Jump(&resume_with_return);
Out of curiosity, do you even need the Jump?

[neis, 2016-04-18 13:58:23]

The CQ bit was checked by neis@chromium.org

[neis, 2016-04-18 13:58:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1884183002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-04-18 13:58:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1884183002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1884183002/120001

[neis, 2016-04-18 14:05:37]

Description was changed from

==========
First version of the new generators implementation.

Behind --ignition-generators. Does not yet support Turbofan.

BUG=v8:4907
LOG=n
==========

to

==========
First version of the new generators implementation.

Behind --ignition-generators. Does not yet support Turbofan.

TBR=bmeurer@chromium.org
BUG=v8:4907
LOG=n
==========

[neis, 2016-04-18 14:07:53]

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:644: resume_points_.clear();
On 2016/04/18 10:01:57, rmcilroy wrote:
> On 2016/04/18 08:04:17, neis wrote:
> > On 2016/04/15 13:17:46, rmcilroy wrote:
> > > Do we need the clear? it should be empty when we reach here anyway.
> > 
> > MakeBytecode could be called several times on the same BytecodeGenerator
> > instance, right?
> > 
> > If that's not supposed to happen, then I don't understand why
CompilationInfo
> is
> > passed to MakeBytecode and not to the BytecodeGenerator constructor.
> 
> No it's only ever used for a single compile info. You are right though, we
> should probably change the constructor to take the CompileInfo, I'll upload a
CL
> to make that change.

Ok. After your change I can probably initialize it directly with the correct
size.

https://codereview.chromium.org/1884183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2273: int id = yield_count_++;
On 2016/04/18 10:01:57, rmcilroy wrote:
> On 2016/04/18 08:04:17, neis wrote:
> > On 2016/04/15 13:17:46, rmcilroy wrote:
> > > Rather than doing this id naming implicitly, would it be safer to have the
> > Yield
> > > AST object include an ID (e.g., the one used for state calculation) which
we
> > use
> > > as the id here and as the index to the resume_points_ ?
> > 
> > I don't think that would be safer and I don't see any advantage. Also, my
> > transformation for dispatching in loops will rely on yields being numbered
> > consecutively.
> 
> OK. I'm fine with this for this CL, but maybe it's worth considering
maintaining
> yield_count_ and resume_points_ in a small class to abstract the yield /
resume
> point maintanance (particularly if the dispatching in loops CL adds more logic
> to this). 

OK.  Also, I probably need to move them into arraybuilder for the next CL.

https://codereview.chromium.org/1884183002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1884183002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:2320: .Jump(&resume_with_return);
On 2016/04/18 13:55:06, Jarin wrote:
> Out of curiosity, do you even need the Jump?

Shhh :)

[commit-bot: I haz the power, 2016-04-18 14:10:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-18 14:10:25]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/13791)

[neis, 2016-04-18 14:10:50]

The CQ bit was checked by neis@chromium.org

[commit-bot: I haz the power, 2016-04-18 14:11:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1884183002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1884183002/120001

[commit-bot: I haz the power, 2016-04-18 14:13:08]

Description was changed from

==========
First version of the new generators implementation.

Behind --ignition-generators. Does not yet support Turbofan.

TBR=bmeurer@chromium.org
BUG=v8:4907
LOG=n
==========

to

==========
First version of the new generators implementation.

Behind --ignition-generators. Does not yet support Turbofan.

TBR=bmeurer@chromium.org
BUG=v8:4907
LOG=n
==========

[commit-bot: I haz the power, 2016-04-18 14:13:10]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-04-18 14:13:38]

Description was changed from

==========
First version of the new generators implementation.

Behind --ignition-generators. Does not yet support Turbofan.

TBR=bmeurer@chromium.org
BUG=v8:4907
LOG=n
==========

to

==========
First version of the new generators implementation.

Behind --ignition-generators. Does not yet support Turbofan.

TBR=bmeurer@chromium.org
BUG=v8:4907
LOG=n

Committed: https://crrev.com/d0ccddd0324aa9391e76b611481bdc47f0f073de
Cr-Commit-Position: refs/heads/master@{#35584}
==========

[commit-bot: I haz the power, 2016-04-18 14:13:40]

Patchset 6 (id:??) landed as
https://crrev.com/d0ccddd0324aa9391e76b611481bdc47f0f073de
Cr-Commit-Position: refs/heads/master@{#35584}