Upstreaming Cygprofile for Android.

Upstreaming Cygprofile for Android.

This Cygprofile is used only by Android and is different from the version in //tools/cygprofile/ .
See base/android/third_party/cygprofile_startup/README.chromium for more information.

BUG=136985
TEST=

[felipeg, 2012-07-10 10:30:56]

Ping

[digit1, 2012-07-10 10:42:59]

https://chromiumcodereview.appspot.com/10697079/diff/1/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/10697079/diff/1/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:1511: #if !defined(OS_ANDROID)
Might want to put a comment here, and/or a link to a crbug entry related to the
lack of extensions in Chrome for Android. I'm not sure this is part of this
specifici issue though, does that prevent startup profiling or is it related to
something different?

[felipeg, 2012-07-10 11:14:53]

Done.

https://chromiumcodereview.appspot.com/10697079/diff/1/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/10697079/diff/1/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:1511: #if !defined(OS_ANDROID)
On 2012/07/10 10:42:59, digit1 wrote:
> Might want to put a comment here, and/or a link to a crbug entry related to
the
> lack of extensions in Chrome for Android. I'm not sure this is part of this
> specifici issue though, does that prevent startup profiling or is it related
to
> something different?


This change shouldn't be here.
I mistakenly reused a git branch.
Let me revert this

[Yaron, 2012-07-10 15:34:30]

felipeg: when sending out a patch to multiple owners/reviewers please indicate
what you want each reviewer to look at (e.g. bulach: base/android, brettw:
base/, etc)

[felipeg, 2012-07-10 15:43:09]

On 2012/07/10 15:34:30, Yaron wrote:
> felipeg: when sending out a patch to multiple owners/reviewers please indicate
> what you want each reviewer to look at (e.g. bulach: base/android, brettw:
> base/, etc)

All right. Sorry.
Here you go:
Please reviewers, review the following:
 bulach: base/android
 brettw: base  build  ppapi

thanks

[felipeg, 2012-07-11 15:58:00]

FYI
I am adding Jay and Steve Block as reviewers.
Jay can approve for base/android
and Steve was in the original thread where we discussed this issue.

[Steve Block, 2012-07-11 16:17:56]

You need a review by open-source-third-party-reviews@google.com - see
http://www.chromium.org/developers/adding-3rd-party-libraries

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/android/third_...
File base/android/third_party/cygprofile_startup/LICENSE (right):

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/android/third_...
base/android/third_party/cygprofile_startup/LICENSE:2: * cyg-profile.h - Header
file for CygProfiler
I think you should remove this and reformat this license text. Maybe mention in
README.chromium that the license text comes from cyg-profile.h.

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/android/third_...
File base/android/third_party/cygprofile_startup/README.chromium (right):

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/android/third_...
base/android/third_party/cygprofile_startup/README.chromium:8: License File:
cyg-profile.h
I don't see this file. Do you mean LICENSE?

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/android/third_...
File base/android/third_party/cygprofile_startup/cygprofile_startup.gyp (right):

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/android/third_...
base/android/third_party/cygprofile_startup/cygprofile_startup.gyp:1: #
Copyright (c) 2011 The Chromium Authors. All rights reserved.
2012

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/android/third_...
File base/android/third_party/cygprofile_startup/cygprofile_startup_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/android/third_...
base/android/third_party/cygprofile_startup/cygprofile_startup_unittest.cc:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
2012

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/base.gyp
File base/base.gyp (right):

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/base.gyp#newco...
base/base.gyp:39: 'conditions': [
Can you collapse this to single condition block?

[felipeg, 2012-07-11 17:10:25]

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/android/third_...
File base/android/third_party/cygprofile_startup/LICENSE (right):

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/android/third_...
base/android/third_party/cygprofile_startup/LICENSE:2: * cyg-profile.h - Header
file for CygProfiler
On 2012/07/11 16:17:56, Steve Block wrote:
> I think you should remove this and reformat this license text. Maybe mention
in
> README.chromium that the license text comes from cyg-profile.h.

Done.

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/android/third_...
File base/android/third_party/cygprofile_startup/README.chromium (right):

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/android/third_...
base/android/third_party/cygprofile_startup/README.chromium:8: License File:
cyg-profile.h
On 2012/07/11 16:17:56, Steve Block wrote:
> I don't see this file. Do you mean LICENSE?

Done.

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/android/third_...
File base/android/third_party/cygprofile_startup/cygprofile_startup.gyp (right):

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/android/third_...
base/android/third_party/cygprofile_startup/cygprofile_startup.gyp:1: #
Copyright (c) 2011 The Chromium Authors. All rights reserved.
On 2012/07/11 16:17:56, Steve Block wrote:
> 2012

Done.

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/android/third_...
File base/android/third_party/cygprofile_startup/cygprofile_startup_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/android/third_...
base/android/third_party/cygprofile_startup/cygprofile_startup_unittest.cc:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
On 2012/07/11 16:17:56, Steve Block wrote:
> 2012

Done.

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/base.gyp
File base/base.gyp (right):

https://chromiumcodereview.appspot.com/10697079/diff/7001/base/base.gyp#newco...
base/base.gyp:39: 'conditions': [
On 2012/07/11 16:17:56, Steve Block wrote:
> Can you collapse this to single condition block?

Done.

[Daniel Berlin, 2012-07-16 15:13:57]

lgtm

[brettw, 2012-07-16 21:27:35]

Why does this need to be added to base?

If Android needs a different version, it seems that we should have it in the
same place and call it cygprofile_android or somesuch. Having different versions
in different parts of the tree seems very confusing to me.

[felipeg_google, 2012-07-17 16:09:34]

Hi Brett,

I agree that it is confusing.
But FYI, the current version used in android sits (downstream) in:
//third_party/cygprofile/
While the version used by chromium sits (upstream) in: //tools/cygprofile/

Would you prefer to have the android version
as //third_party/cygprofile_android/
While keeping the chromium version at //tools/cygprofile/ ?

As opposed to the current CL that moves it
to //base/android/third_party/cygprofile_statup .

I can do that if you think it is better.
We don't have a strong reason to move it to base/android/
We just thought that base/android/ would be the best fit for cygprofile/

cheers,
Felipeg

On Mon, Jul 16, 2012 at 11:27 PM, <brettw@chromium.org> wrote:

> Why does this need to be added to base?
>
> If Android needs a different version, it seems that we should have it in
> the
> same place and call it cygprofile_android or somesuch. Having different
> versions
> in different parts of the tree seems very confusing to me.
>
>
https://chromiumcodereview.**appspot.com/10697079/<https://chromiumcodereview...
>

[brettw, 2012-07-17 17:02:36]

This cygprofile you're checking in, is it from the Android tree or is it
something the Chrome-on-Android team forked from somewhere?

[felipeg_google, 2012-07-17 21:04:49]

On Tue, Jul 17, 2012 at 7:02 PM, <brettw@chromium.org> wrote:

> This cygprofile you're checking in, is it from the Android tree or is it
> something the Chrome-on-Android team forked from somewhere?
>

Chrome-on-Android team forked from the original Cygprofile C code.
But the version we have now has so many changes that it doesn't resemble
the original code.


>
https://chromiumcodereview.**appspot.com/10697079/<https://chromiumcodereview...
>

[brettw, 2012-07-17 23:20:03]

I'm still having some problems understanding why we need two copies of this
stuff. It seems really like we should have one copy that does what we need or
can be controlled such that it logs the first time rather than every time.

I don't understand what the CL description means by "extensions". cygprofile.cc
in Chrome looks like it has no dependencies except on Base.

[felipeg_google, 2012-07-18 09:13:25]

Hey Brett, I understand your point, let me put this on hold for now.
I will talk to David Turner, who has been using the downstream version of
cygprofile. He is in vacation and should be back in 2 weeks.
He may have a better idea of how to merge these changes into the one we
have upstream or come up with some other plan.

The "extensions" mention in the CL description was actually a copy&paste
mistake of mine. It came from another CL was upstreaming.


On Wed, Jul 18, 2012 at 1:20 AM, <brettw@chromium.org> wrote:

> I'm still having some problems understanding why we need two copies of this
> stuff. It seems really like we should have one copy that does what we need
> or
> can be controlled such that it logs the first time rather than every time.
>
> I don't understand what the CL description means by "extensions".
> cygprofile.cc
> in Chrome looks like it has no dependencies except on Base.
>
>
https://chromiumcodereview.**appspot.com/10697079/<https://chromiumcodereview...
>

[felipeg, 2012-07-18 16:36:02]

Hi Brett,

I just got another idea for a solution.
Instead of creating a new third_party directory under base/android/
I am moving the version of cygprofile used in android to //tools/cygprofile and
renaming it to cygprofile_android.h as you suggested.
This way both versions (android and chromium) are in the same path
(//tools/cygprofile), but in different files (different implementations)
cygprofile.cc for chromium and cygprofile_android.cc for android.

What you think ?



On 2012/07/18 09:13:25, felipeg wrote:
> Hey Brett, I understand your point, let me put this on hold for now.
> I will talk to David Turner, who has been using the downstream version of
> cygprofile. He is in vacation and should be back in 2 weeks.
> He may have a better idea of how to merge these changes into the one we
> have upstream or come up with some other plan.
> 
> The "extensions" mention in the CL description was actually a copy&paste
> mistake of mine. It came from another CL was upstreaming.
> 
> 
> On Wed, Jul 18, 2012 at 1:20 AM, <mailto:brettw@chromium.org> wrote:
> 
> > I'm still having some problems understanding why we need two copies of this
> > stuff. It seems really like we should have one copy that does what we need
> > or
> > can be controlled such that it logs the first time rather than every time.
> >
> > I don't understand what the CL description means by "extensions".
> > cygprofile.cc
> > in Chrome looks like it has no dependencies except on Base.
> >
> >
>
https://chromiumcodereview.**appspot.com/10697079/%3Chttps://chromiumcoderevi...>
> >

[brettw, 2012-07-18 18:07:21]

That suggestion is better but it still doesn't answer the question of why we
need two copies of this code.

[felipeg_google, 2012-07-18 19:25:13]

I believe we need two copies of that code because of the following reasons:

 - Android cygprofile code has been rewritten in C++
 - It uses a completely different format for logging that matches the
parser we use for chrome android code positioning analysis (used for
building link maps to improve instruction cache hits and startup times)
 - It uses a stl::set to keep track of functions that has already been
called so that it only generate logs for the first call of each function.

The above mentioned differences could be refactored into one single
implementation file using IFDEFs or run time IFs.
But I believe that the difference is big enough to justify keeping both
files separate.



On Wed, Jul 18, 2012 at 8:07 PM, <brettw@chromium.org> wrote:

> That suggestion is better but it still doesn't answer the question of why
> we
> need two copies of this code.
>
>
https://chromiumcodereview.**appspot.com/10697079/<https://chromiumcodereview...
>

[felipeg, 2012-07-23 13:58:00]

Ping: Brettw.

Thanks,
Felipeg

On 2012/07/18 19:25:13, felipeg wrote:
> I believe we need two copies of that code because of the following reasons:
> 
>  - Android cygprofile code has been rewritten in C++
>  - It uses a completely different format for logging that matches the
> parser we use for chrome android code positioning analysis (used for
> building link maps to improve instruction cache hits and startup times)
>  - It uses a stl::set to keep track of functions that has already been
> called so that it only generate logs for the first call of each function.
> 
> The above mentioned differences could be refactored into one single
> implementation file using IFDEFs or run time IFs.
> But I believe that the difference is big enough to justify keeping both
> files separate.
> 
> 
> 
> On Wed, Jul 18, 2012 at 8:07 PM, <mailto:brettw@chromium.org> wrote:
> 
> > That suggestion is better but it still doesn't answer the question of why
> > we
> > need two copies of this code.
> >
> >
>
https://chromiumcodereview.**appspot.com/10697079/%3Chttps://chromiumcoderevi...>
> >

[brettw, 2012-07-23 21:08:53]

On Wed, Jul 18, 2012 at 12:25 PM, Felipe Goldstein <felipeg@google.com>wrote:

> I believe we need two copies of that code because of the following reasons:
>
>  - Android cygprofile code has been rewritten in C++
>

I don't understand why this is a justification for doing anything. If
you're arguing that the Android code is better then I think we should use
it for everything and delete the old one.


 - It uses a completely different format for logging that matches the
> parser we use for chrome android code positioning analysis (used for
> building link maps to improve instruction cache hits and startup times)
>
 - It uses a stl::set to keep track of functions that has already been
> called so that it only generate logs for the first call of each function.
>


It seems like this should be an option. For example, for Chrome logging, we
have the ability to specify whether different fields are formatted. Imagine
if I said I wanted to have a different implementation of base/logging.cc
because the output format was different, or because I only wanted to log
the first time. That would seem silly and would make it confusing for
everybody.

Brett

[felipeg_google, 2012-07-26 15:53:46]

On Mon, Jul 23, 2012 at 11:08 PM, Brett Wilson <brettw@chromium.org> wrote:

> On Wed, Jul 18, 2012 at 12:25 PM, Felipe Goldstein <felipeg@google.com>wrote:
>
>> I believe we need two copies of that code because of the following
>> reasons:
>>
>>  - Android cygprofile code has been rewritten in C++
>>
>
> I don't understand why this is a justification for doing anything. If
> you're arguing that the Android code is better then I think we should use
> it for everything and delete the old one.
>
>
>  - It uses a completely different format for logging that matches the
>> parser we use for chrome android code positioning analysis (used for
>> building link maps to improve instruction cache hits and startup times)
>>
>  - It uses a stl::set to keep track of functions that has already been
>> called so that it only generate logs for the first call of each function.
>>
>
>
> It seems like this should be an option. For example, for Chrome logging,
> we have the ability to specify whether different fields are formatted.
> Imagine if I said I wanted to have a different implementation of
> base/logging.cc because the output format was different, or because I only
> wanted to log the first time. That would seem silly and would make it
> confusing for everybody.
>


Hi Brett,
I totally buy your argument above.
I will try to merge both implementations into one single file.
But since it is not urgent, it may take a while.
Do you think it is better to drop this CL and do another one after, or just
keep it and do the change here ?



>
> Brett
>

[brettw, 2012-07-26 17:25:04]

Whatever is more convenient for you is fine here.

Brett

[brettw, 2012-07-26 17:25:16]

BTW thanks for working on this.

Brett

[digit1, 2012-07-30 11:50:50]

Now that I'm back from vacation, I'll try to answer brett's question about why
this different version is needed.

The cygprofile_android.cc implementation (which is much older than the one in
chromium) is only used to record the timestamp of the _first_ call of any
function during startup.

  - the corresponding data is stored on a file on the device, which typically
has limited capacity.

  - it is later extracted, and processed by helper scripts (not available in
this patch), which end up generating a new linker map file that optimizes the
placement of functions in the final binary to speed up startup.

By contrast, the chromium version records the timestamps of all function entries
and exits.

  - this is a *lot* more data, that we don't need and don't want to store on the
device's file system.

  - the data is also in a completely different format, which means we would have
to change our scripts significantly to process them.

All other aspects (C++ + using a std::set) are implementation details that could
be removed / changed / optimized. But the main reason to have two distinct
versions comes from the fact that we record sufficiently different things.

Maybe renaming this to android_startup_profiler.cc would help make this
distinction more explicit.

Hope this helps.

[brettw, 2012-07-31 21:09:04]

Both the Linux & Android implementations are ~350 lines. If somebody came to me
and said "I'm writing a separate 350 line function because I only want to mark
the first time I see X rather than every time" and I have a different output
format, I would smack them and make them unify the implementations and I think
you would too.

I don't understand why this was forked in the first place. If you want to record
only the first time something happens, it seems like that's a 5 line change. If
you want to change the output format, that's also OK with me, have a different
output module or options or something.

But nothing that's been said makes any sense to me as a random 3rd party about
why we need to implementations of this in our source code. And if it doesn't
make sense to me, it won't make sense to any of the engineers for the next 20
years who have to touch this code.

[digit1, 2012-08-02 10:52:17]

On 2012/07/31 21:09:04, brettw wrote:
> Both the Linux & Android implementations are ~350 lines. If somebody came to
me
> and said "I'm writing a separate 350 line function because I only want to mark
> the first time I see X rather than every time" and I have a different output
> format, I would smack them and make them unify the implementations and I think
> you would too.
> 

otoh, I fear you're going to end up with a 700-lines function with more
configuration knobs, and more subtle maintenance headaches.

> I don't understand why this was forked in the first place.

It was not "forked", it was written a long time before there was a cygprofile in
the Chromium tree, as far as I remember.

 If you want to record
> only the first time something happens, it seems like that's a 5 line change.
If
> you want to change the output format, that's also OK with me, have a different
> output module or options or something.
> 

There are actually another detail I forgot. Unlike the original upstream
cygprofile code, we want to start recording as soon as the first function call
is profiled. This is necessary to record static C++ constructors being called
before any other function the shared library (we used to call
cygprofile_enable() later, missing some stuff).

To do this in a thread-safe way, we had to use a pthread_once_t. Note sure how
this would translate to a more generic Chromium cygprofile source that needs to
also run on Windows, I guess.

> But nothing that's been said makes any sense to me as a random 3rd party about
> why we need to implementations of this in our source code. And if it doesn't
> make sense to me, it won't make sense to any of the engineers for the next 20
> years who have to touch this code.

[brettw, 2012-08-02 18:23:42]

Felipe said it was forked which gave me that impression. Perhaps he
was misinformed, or is there something else that both the Chrome and
Android versions were both forked from? Can you clarify this?

I don't actually care that much about this code. I just think the
situation is confusing and I haven't gotten a super clear
justification for the current state of affairs.

The original patch was trying to land in a third_party directory. Just
to clarify, this is our code and what we check in will be the
canonical location for this file? Or is this copied from somewhere
else that we need to keep in sync with?

Brett

[Steve Block, 2012-08-03 16:53:22]

https://chromiumcodereview.appspot.com/10697079/diff/25002/tools/cygprofile/L...
File tools/cygprofile/LICENSE (right):

https://chromiumcodereview.appspot.com/10697079/diff/25002/tools/cygprofile/L...
tools/cygprofile/LICENSE:5: See cygprofile_android.cc for details on usage.
This shouldn't be part of the license file.

https://chromiumcodereview.appspot.com/10697079/diff/25002/tools/cygprofile/R...
File tools/cygprofile/README (right):

https://chromiumcodereview.appspot.com/10697079/diff/25002/tools/cygprofile/R...
tools/cygprofile/README:1: CygProfiler suite
Is this file taken from upstream Cygprofile? If not, you could merge it with
README.chromium. Also, does it apply to both Android and 'regular' versions?

https://chromiumcodereview.appspot.com/10697079/diff/25002/tools/cygprofile/R...
File tools/cygprofile/README.chromium (right):

https://chromiumcodereview.appspot.com/10697079/diff/25002/tools/cygprofile/R...
tools/cygprofile/README.chromium:9: Security Critical: No
Thanks for providing this metadata. Please add this directory to
ADDITIONAL_PATHS in tools/licenses.py to make sure it stays correct (see the
output of 'tools/licenses.py scan'.)

https://chromiumcodereview.appspot.com/10697079/diff/25002/tools/cygprofile/R...
tools/cygprofile/README.chromium:15: Local Modifications:
Do these apply to the Android or 'regular' versions?

https://chromiumcodereview.appspot.com/10697079/diff/25002/tools/cygprofile/R...
tools/cygprofile/README.chromium:42: 07/18/2012: Renaming and moving
//base/android/third_party/cygprofile_startup to
This history of file locations etc refers to the downstream Android tree, right?
I think this will be very confusing to a future Chromium developer, as they
won't be able to see any of this history. It's OK to describe the Android
history in the CL comments, but I think that for the purposes of this README, we
should consider the origin of the Android code to be this CL.