android: Remove command line args from intent bundle

android: Remove command line args from intent bundle

The intent can be saved by android and re-used for launching a child
service in edge cases. However the command line arguments are different
for each launch of the child process. The command line args are already
passed through an additional bundle through the aidl interface. So just
fallback to the aidl bundle and remove command line args from the intent
bundle.

Note this may and probably does decrease parallelism in child service
start up, thus causing a perf regression.

BUG=664341
Committed: https://crrev.com/74a2272cac57b1b267ab8e2c1390c2010f58b063
Cr-Commit-Position: refs/heads/master@{#437929}

[boliu, 2016-12-10 17:16:35]

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

[commit-bot: I haz the power, 2016-12-10 17:16:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[boliu, 2016-12-10 17:19:58]

The CQ bit was unchecked by boliu@chromium.org

[boliu, 2016-12-10 17:58:36]

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

[commit-bot: I haz the power, 2016-12-10 17:58:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 19:02:16]

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

[commit-bot: I haz the power, 2016-12-10 19:02:17]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[boliu, 2016-12-12 17:25:10]

boliu@chromium.org changed reviewers:
+ agrieve@chromium.org, mariakhomenko@chromium.org

[boliu, 2016-12-12 17:25:11]

ptal

randomization does not appear to help. but this is the real fix, and it's only
mostly removing code, so seems pretty safe to me?

More context: https://bugs.chromium.org/p/chromium/issues/detail?id=664341#c92

[Jay Civelli, 2016-12-12 17:40:27]

jcivelli@chromium.org changed reviewers:
+ jcivelli@chromium.org

[Jay Civelli, 2016-12-12 17:40:28]

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(left):

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:291:
mLinkerParams = new ChromiumLinkerParams(intent);
In the future, should we still depend on anything from the intent at all?

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:168:
while (!mIsBound) {
I guess now we'll always be bound by the time we get there?

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:300:
// Allow the command line to be set via bind() intent or setupConnection, but
Is this comment still accurate?

[boliu, 2016-12-12 17:54:02]

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(left):

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:291:
mLinkerParams = new ChromiumLinkerParams(intent);
On 2016/12/12 17:40:28, Jay Civelli wrote:
> In the future, should we still depend on anything from the intent at all?

I dunno.. it's still ok to pass things that's fixed for the entire installation.
And if linker args fall into that category, then it's a win on start up time

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:168:
while (!mIsBound) {
On 2016/12/12 17:40:28, Jay Civelli wrote:
> I guess now we'll always be bound by the time we get there?

Yeah afaict, so should be safe to remove.

And removing should make the regression slightly less (though I guess probably
not by much).

Others got opinion whether removing this is safe?

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:300:
// Allow the command line to be set via bind() intent or setupConnection, but
On 2016/12/12 17:40:28, Jay Civelli wrote:
> Is this comment still accurate?

I dunno what this comment is really saying.. bind always comes first afaik, so
it's just saying command line args is (was) in bind, but transferring FDs has to
be here (obviously..)

Uhh... just remove it?

[boliu, 2016-12-12 18:13:08]

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:168:
while (!mIsBound) {
On 2016/12/12 17:54:02, boliu wrote:
> On 2016/12/12 17:40:28, Jay Civelli wrote:
> > I guess now we'll always be bound by the time we get there?
> 
> Yeah afaict, so should be safe to remove.

Actually, I can't find any hard evidence to back this up.

If onBind *always* happens before setupConnection, then this mIsBound check is
not needed even without this CL, which leads me to think when digit@ first wrote
this, that setupConnection before bind *is* possible.

So to be cautious for merging, let's not touch this..

> 
> And removing should make the regression slightly less (though I guess probably
> not by much).
> 
> Others got opinion whether removing this is safe?

[boliu, 2016-12-12 18:21:23]

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:168:
while (!mIsBound) {
On 2016/12/12 18:13:08, boliu wrote:
> On 2016/12/12 17:54:02, boliu wrote:
> > On 2016/12/12 17:40:28, Jay Civelli wrote:
> > > I guess now we'll always be bound by the time we get there?
> > 
> > Yeah afaict, so should be safe to remove.
> 
> Actually, I can't find any hard evidence to back this up.

Oh, bahh, I can't decide.. Here's the evidence:
bind returns the aidl interface that browser then uses to call setupConnection,
so bind has to happen first.

Also nothing ever calls ChildProcessService.initializeParams directly, so the
only callsite of initializeParams is from bind.

> 
> If onBind *always* happens before setupConnection, then this mIsBound check is
> not needed even without this CL, which leads me to think when digit@ first
wrote
> this, that setupConnection before bind *is* possible.
> 
> So to be cautious for merging, let's not touch this..
> 
> > 
> > And removing should make the regression slightly less (though I guess
probably
> > not by much).
> > 
> > Others got opinion whether removing this is safe?

[Jay Civelli, 2016-12-12 18:41:16]

On Mon, Dec 12, 2016 at 10:21 AM, <boliu@chromium.org> wrote:

>
> https://codereview.chromium.org/2560403002/diff/1/content/
> public/android/java/src/org/chromium/content/app/
> ChildProcessServiceImpl.java
> File
> content/public/android/java/src/org/chromium/content/app/
> ChildProcessServiceImpl.java
> (right):
>
> https://codereview.chromium.org/2560403002/diff/1/content/
> public/android/java/src/org/chromium/content/app/
> ChildProcessServiceImpl.java#newcode168
> content/public/android/java/src/org/chromium/content/app/
> ChildProcessServiceImpl.java:168:
> while (!mIsBound) {
> On 2016/12/12 18:13:08, boliu wrote:
> > On 2016/12/12 17:54:02, boliu wrote:
> > > On 2016/12/12 17:40:28, Jay Civelli wrote:
> > > > I guess now we'll always be bound by the time we get there?
> > >
> > > Yeah afaict, so should be safe to remove.
> >
> > Actually, I can't find any hard evidence to back this up.
>
> Oh, bahh, I can't decide.. Here's the evidence:
> bind returns the aidl interface that browser then uses to call
> setupConnection, so bind has to happen first.
>
> Also nothing ever calls ChildProcessService.initializeParams directly,
> so the only callsite of initializeParams is from bind.
>

Yeah, that was my thought too.


>
>
> >
> > If onBind *always* happens before setupConnection, then this mIsBound
> check is
> > not needed even without this CL, which leads me to think when digit@
> first wrote
> > this, that setupConnection before bind *is* possible.
> >
> > So to be cautious for merging, let's not touch this..
> >
> > >
> > > And removing should make the regression slightly less (though I
> guess probably
> > > not by much).
> > >
> > > Others got opinion whether removing this is safe?
>
> https://codereview.chromium.org/2560403002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[boliu, 2016-12-12 18:42:14]

Patchset #2 (id:20001) has been deleted

[boliu, 2016-12-12 18:52:15]

ok, deleted mIsBound, and some related stuff

[boliu, 2016-12-12 18:52:23]

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

[commit-bot: I haz the power, 2016-12-12 18:53:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[agrieve, 2016-12-12 18:59:21]

lgtm afaict :)

[Jay Civelli, 2016-12-12 19:11:22]

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(left):

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:291:
mLinkerParams = new ChromiumLinkerParams(intent);
On 2016/12/12 17:54:02, boliu wrote:
> On 2016/12/12 17:40:28, Jay Civelli wrote:
> > In the future, should we still depend on anything from the intent at all?
> 
> I dunno.. it's still ok to pass things that's fixed for the entire
installation.
> And if linker args fall into that category, then it's a win on start up time
Yes, but then the linker has to wait for the command line to be set iiuc. I
think it would be less error prone to always pass everything in the IPC call.
Not something you need to do there, but maybe add a TODO. When I'll refactor
this code as part of the servicification effort, I'll take a look at this.

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:300:
// Allow the command line to be set via bind() intent or setupConnection, but
On 2016/12/12 17:54:02, boliu wrote:
> On 2016/12/12 17:40:28, Jay Civelli wrote:
> > Is this comment still accurate?
> 
> I dunno what this comment is really saying.. bind always comes first afaik, so
> it's just saying command line args is (was) in bind, but transferring FDs has
to
> be here (obviously..)
> 
> Uhh... just remove it?

Yes, maybe remove since now there is only one way to set the cmd line now.

[boliu, 2016-12-12 19:16:02]

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(left):

https://codereview.chromium.org/2560403002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:291:
mLinkerParams = new ChromiumLinkerParams(intent);
On 2016/12/12 19:11:22, Jay Civelli wrote:
> On 2016/12/12 17:54:02, boliu wrote:
> > On 2016/12/12 17:40:28, Jay Civelli wrote:
> > > In the future, should we still depend on anything from the intent at all?
> > 
> > I dunno.. it's still ok to pass things that's fixed for the entire
> installation.
> > And if linker args fall into that category, then it's a win on start up time
> Yes, but then the linker has to wait for the command line to be set iiuc. I
> think it would be less error prone to always pass everything in the IPC call.

Surely we can refactor the linker to not require command line args? which of
course is out of scope for this CL. But that's my plan if perf bots complain.

> Not something you need to do there, but maybe add a TODO. When I'll refactor
> this code as part of the servicification effort, I'll take a look at this.

[Maria, 2016-12-12 19:30:24]

lgtm

[boliu, 2016-12-12 21:02:16]

The CQ bit was unchecked by boliu@chromium.org

[boliu, 2016-12-12 21:02:20]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2016-12-12 21:05:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 22:20:01]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481576540042680,
"parent_rev": "8e9799bdc31338a84c7f16537b8341cbfbd0c3f3", "commit_rev":
"8f8bbf6dcd46f0d54fd53f781d216310e3446452"}

[commit-bot: I haz the power, 2016-12-12 22:20:28]

Description was changed from

==========
android: Remove command line args from intent bundle

The intent can be saved by android and re-used for launching a child
service in edge cases. However the command line arguments are different
for each launch of the child process. The command line args are already
passed through an additional bundle through the aidl interface. So just
fallback to the aidl bundle and remove command line args from the intent
bundle.

Note this may and probably does decrease parallelism in child service
start up, thus causing a perf regression.

BUG=664341
==========

to

==========
android: Remove command line args from intent bundle

The intent can be saved by android and re-used for launching a child
service in edge cases. However the command line arguments are different
for each launch of the child process. The command line args are already
passed through an additional bundle through the aidl interface. So just
fallback to the aidl bundle and remove command line args from the intent
bundle.

Note this may and probably does decrease parallelism in child service
start up, thus causing a perf regression.

BUG=664341

Review-Url: https://codereview.chromium.org/2560403002
==========

[commit-bot: I haz the power, 2016-12-12 22:20:29]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-12-12 22:22:42]

Description was changed from

==========
android: Remove command line args from intent bundle

The intent can be saved by android and re-used for launching a child
service in edge cases. However the command line arguments are different
for each launch of the child process. The command line args are already
passed through an additional bundle through the aidl interface. So just
fallback to the aidl bundle and remove command line args from the intent
bundle.

Note this may and probably does decrease parallelism in child service
start up, thus causing a perf regression.

BUG=664341

Review-Url: https://codereview.chromium.org/2560403002
==========

to

==========
android: Remove command line args from intent bundle

The intent can be saved by android and re-used for launching a child
service in edge cases. However the command line arguments are different
for each launch of the child process. The command line args are already
passed through an additional bundle through the aidl interface. So just
fallback to the aidl bundle and remove command line args from the intent
bundle.

Note this may and probably does decrease parallelism in child service
start up, thus causing a perf regression.

BUG=664341

Committed: https://crrev.com/74a2272cac57b1b267ab8e2c1390c2010f58b063
Cr-Commit-Position: refs/heads/master@{#437929}
==========

[commit-bot: I haz the power, 2016-12-12 22:22:43]

Patchset 3 (id:??) landed as
https://crrev.com/74a2272cac57b1b267ab8e2c1390c2010f58b063
Cr-Commit-Position: refs/heads/master@{#437929}