Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11348354:
Change cygprofile to work on Android. (Closed)
|
Created:
8 years ago by felipeg Modified:
8 years ago CC:
chromium-reviews, pam+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionChange cygprofile to work on Android.
We want to use cygprofile for the same purpose as the chromeos: To generate order_text_session files based on earliest run of each function call.
BUG=150893
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171034
Patch Set 1 #
Total comments: 8
Patch Set 2 : #Patch Set 3 : #
Total comments: 7
Messages
Total messages: 17 (0 generated)
https://chromiumcodereview.appspot.com/11348354/diff/1/build/common.gypi File build/common.gypi (right): https://chromiumcodereview.appspot.com/11348354/diff/1/build/common.gypi#newc... build/common.gypi:2743: ['order_profiling!=0 and (chromeos==1 or OS=="linux" or OS=="android")', { 80-columns exceeded https://chromiumcodereview.appspot.com/11348354/diff/1/tools/cygprofile/cygpr... File tools/cygprofile/cygprofile.cc (right): https://chromiumcodereview.appspot.com/11348354/diff/1/tools/cygprofile/cygpr... tools/cygprofile/cygprofile.cc:290: #if !defined(OS_ANDROID) Could you please make a simple test that verifies that cygprofile still records all routines in a simple multi-threaded program for OS_ANDROID.
nice, thanks felipe! I'm not too familiar with this part, but go ahead once glotov is happy.. just one suggestion below: https://chromiumcodereview.appspot.com/11348354/diff/1/tools/cygprofile/cygpr... File tools/cygprofile/cygprofile.cc (right): https://chromiumcodereview.appspot.com/11348354/diff/1/tools/cygprofile/cygpr... tools/cygprofile/cygprofile.cc:167: #if defined(OS_ANDROID) maybe splitting this would be nicer: #if defined(OS_ANDROID) const char CytTlsLog::kLogFileNamePrefix = "/data/local/tmp/chrome/cyglog/cyglog."; #else const char CytTlsLog::kLogFileNamePrefix = "/var/log/chrome/cyglog."; #endif const char CygTlsLog::kLogFilenameFmt[] = "%s%d.%d.%ld-%d" this way if the format changes we'd need to change just one place.. wdyt?
https://chromiumcodereview.appspot.com/11348354/diff/1/build/common.gypi File build/common.gypi (right): https://chromiumcodereview.appspot.com/11348354/diff/1/build/common.gypi#newc... build/common.gypi:2743: ['order_profiling!=0 and (chromeos==1 or OS=="linux" or OS=="android")', { On 2012/12/03 16:42:17, glotov wrote: > 80-columns exceeded Currently this file has several places where lines are above 80 cols , for example at line 1942. I think it is standard practice to not break lines. Otherwise, do you know how do I break the line in this case ? (I am not very familiar with Gyp syntax) https://chromiumcodereview.appspot.com/11348354/diff/1/tools/cygprofile/cygpr... File tools/cygprofile/cygprofile.cc (right): https://chromiumcodereview.appspot.com/11348354/diff/1/tools/cygprofile/cygpr... tools/cygprofile/cygprofile.cc:167: #if defined(OS_ANDROID) On 2012/12/03 16:48:30, bulach wrote: > maybe splitting this would be nicer: > > #if defined(OS_ANDROID) > const char CytTlsLog::kLogFileNamePrefix = > "/data/local/tmp/chrome/cyglog/cyglog."; > #else > const char CytTlsLog::kLogFileNamePrefix = "/var/log/chrome/cyglog."; > #endif > > const char CygTlsLog::kLogFilenameFmt[] = "%s%d.%d.%ld-%d" > > this way if the format changes we'd need to change just one place.. wdyt? Done. https://chromiumcodereview.appspot.com/11348354/diff/1/tools/cygprofile/cygpr... tools/cygprofile/cygprofile.cc:290: #if !defined(OS_ANDROID) On 2012/12/03 16:42:17, glotov wrote: > Could you please make a simple test that verifies that cygprofile still records > all routines in a simple multi-threaded program for OS_ANDROID. Done.
https://chromiumcodereview.appspot.com/11348354/diff/1/tools/cygprofile/cygpr... File tools/cygprofile/cygprofile.cc (right): https://chromiumcodereview.appspot.com/11348354/diff/1/tools/cygprofile/cygpr... tools/cygprofile/cygprofile.cc:290: #if !defined(OS_ANDROID) Well, actually it is up to you. Are you 100% sure that android does not use fork (internally) for creating child processes? If so, please add the comment to your #if. Without these hooks child process will produce incorrect log (see comments for AtForkChild), but you may not notice it at first sight. That's why I would create a test. What do you think?
https://chromiumcodereview.appspot.com/11348354/diff/1/tools/cygprofile/cygpr... File tools/cygprofile/cygprofile.cc (right): https://chromiumcodereview.appspot.com/11348354/diff/1/tools/cygprofile/cygpr... tools/cygprofile/cygprofile.cc:290: #if !defined(OS_ANDROID) On 2012/12/03 17:10:08, glotov wrote: > Well, actually it is up to you. Are you 100% sure that android does not use fork > (internally) for creating child processes? If so, please add the comment to your > #if. > > Without these hooks child process will produce incorrect log (see comments for > AtForkChild), but you may not notice it at first sight. That's why I would > create a test. What do you think? I am 100% sure that we don't fork and we will never fork on chrome for android. See the added comments. Thanks for the heads up about this issue.
lgtm https://chromiumcodereview.appspot.com/11348354/diff/4002/tools/cygprofile/cy... File tools/cygprofile/cygprofile.cc (right): https://chromiumcodereview.appspot.com/11348354/diff/4002/tools/cygprofile/cy... tools/cygprofile/cygprofile.cc:37: #include "base/hash_tables.h" BTW, why is this change? https://chromiumcodereview.appspot.com/11348354/diff/4002/tools/cygprofile/cy... tools/cygprofile/cygprofile.cc:297: // will generate its own logs that will later have to be merged as usual. Then how does android create chrome sub-processes (renderers, zygotes)?
https://chromiumcodereview.appspot.com/11348354/diff/4002/tools/cygprofile/cy... File tools/cygprofile/cygprofile.cc (right): https://chromiumcodereview.appspot.com/11348354/diff/4002/tools/cygprofile/cy... tools/cygprofile/cygprofile.cc:297: // will generate its own logs that will later have to be merged as usual. On 2012/12/03 19:28:33, glotov wrote: > Then how does android create chrome sub-processes (renderers, zygotes)? We ask the system to create the processes for us. There is no Chromium zygote, instead, the Dalvik zygote will fork to start any process we need. All this happens before any Chromium native code runs, hence there is no need for pthread_atfork(). Generally speaking, using fork() is unsupported under Android. While it works, the corresponding processes can be killed any time by the system, which doesn't like processes it doesn't have proper accounting for. The browser process, and up to five renderer processes are all described in the AndroidManifest.xml that comes with the application. Also, using the system allows our render processes to run with additional sandboxing under JellyBean (4.1) and higher, i.e. these processes run in a separate UID that is not associated to any installed application. As such, they have absolutely zero permissions (and can't even access the filesystem). The system also allows us to send a few file descriptor to setup IPC channels with them, and that's how things roll... It also means we have to reload libchromeview.so in each process, but that's for another time :)
On 2012/12/03 20:10:38, digit1 wrote: > https://chromiumcodereview.appspot.com/11348354/diff/4002/tools/cygprofile/cy... > File tools/cygprofile/cygprofile.cc (right): > > https://chromiumcodereview.appspot.com/11348354/diff/4002/tools/cygprofile/cy... > tools/cygprofile/cygprofile.cc:297: // will generate its own logs that will > later have to be merged as usual. > On 2012/12/03 19:28:33, glotov wrote: > > Then how does android create chrome sub-processes (renderers, zygotes)? > > We ask the system to create the processes for us. How does the system do it? If it is linux-based, it forks, no? > There is no Chromium zygote, > instead, the Dalvik zygote will fork to start any process we need. All this > happens before any Chromium native code runs, hence there is no need for > pthread_atfork(). Yes, in this case, atfork hooks are not needed. So you start all processes in advance, if I got you right. How do you handle user opening a new tab if you do not have renderer for it? > > Generally speaking, using fork() is unsupported under Android. While it works, > the corresponding processes can be killed any time by the system, which doesn't > like processes it doesn't have proper accounting for. The browser process, and > up to five renderer processes are all described in the AndroidManifest.xml that > comes with the application. > > Also, using the system allows our render processes to run with additional > sandboxing under JellyBean (4.1) and higher, i.e. these processes run in a > separate UID that is not associated to any installed application. As such, they > have absolutely zero permissions (and can't even access the filesystem). The > system also allows us to send a few file descriptor to setup IPC channels with > them, and that's how things roll... > > It also means we have to reload libchromeview.so in each process, but that's for > another time :)
Hey @glotov, I just have run chrome on android with the cygprofile activated and it generated a whole lot of logs (more than 30 files) just for starting up the binary, which I took as a proof that it is being called on every thread. After that I used your pythonscripts to generate the order file. Since on android all the processes are started as separate processes from the beginning (no fork is used), then we can safely strip out the pthread_at_fork call. Would you like me to do any further tests ? On Mon, Dec 3, 2012 at 5:42 PM, <glotov@chromium.org> wrote: > a simple te
thanks glotov! some more details and links below, hope they'll help clarify the architecture here.. On 2012/12/04 07:32:26, glotov wrote: > On 2012/12/03 20:10:38, digit1 wrote: > > > https://chromiumcodereview.appspot.com/11348354/diff/4002/tools/cygprofile/cy... > > File tools/cygprofile/cygprofile.cc (right): > > > > > https://chromiumcodereview.appspot.com/11348354/diff/4002/tools/cygprofile/cy... > > tools/cygprofile/cygprofile.cc:297: // will generate its own logs that will > > later have to be merged as usual. > > On 2012/12/03 19:28:33, glotov wrote: > > > Then how does android create chrome sub-processes (renderers, zygotes)? > > > > We ask the system to create the processes for us. > > How does the system do it? If it is linux-based, it forks, no? afaict, it forks, but not _our_ process.. :) it forks its own zygote (android's, not chromium's), then it loads the java classes for the app (or service, in the renderer's case), then it calls the entry point in java... from there, our java code may then call into native.. so there's no "fork()" exposed / called by the app, if that makes any sense? these may be useful: http://developer.android.com/guide/components/processes-and-threads.html http://developer.android.com/guide/components/services.html > > > There is no Chromium zygote, > > instead, the Dalvik zygote will fork to start any process we need. All this > > happens before any Chromium native code runs, hence there is no need for > > pthread_atfork(). > > Yes, in this case, atfork hooks are not needed. So you start all processes in > advance, if I got you right. How do you handle user opening a new tab if you do > not have renderer for it? we ask the system to start another "service" as above. > > > > > Generally speaking, using fork() is unsupported under Android. While it works, > > the corresponding processes can be killed any time by the system, which > doesn't > > like processes it doesn't have proper accounting for. The browser process, and > > up to five renderer processes are all described in the AndroidManifest.xml > that > > comes with the application. > > > > Also, using the system allows our render processes to run with additional > > sandboxing under JellyBean (4.1) and higher, i.e. these processes run in a > > separate UID that is not associated to any installed application. As such, > they > > have absolutely zero permissions (and can't even access the filesystem). The > > system also allows us to send a few file descriptor to setup IPC channels with > > them, and that's how things roll... > > > > It also means we have to reload libchromeview.so in each process, but that's > for > > another time :)
Hi Glotov@ Although you already LGTM'd , I will wait for another review round from you. Cheers, https://chromiumcodereview.appspot.com/11348354/diff/4002/tools/cygprofile/cy... File tools/cygprofile/cygprofile.cc (right): https://chromiumcodereview.appspot.com/11348354/diff/4002/tools/cygprofile/cy... tools/cygprofile/cygprofile.cc:37: #include "base/hash_tables.h" On 2012/12/03 19:28:33, glotov wrote: > BTW, why is this change? Because we can't yet compile c++0x code for android right now (the sdk doesn't support yet). So we can't use the <unordered_set>
https://chromiumcodereview.appspot.com/11348354/diff/4002/tools/cygprofile/cy... File tools/cygprofile/cygprofile.cc (right): https://chromiumcodereview.appspot.com/11348354/diff/4002/tools/cygprofile/cy... tools/cygprofile/cygprofile.cc:37: #include "base/hash_tables.h" I assume these are equivalent, right? On 2012/12/04 12:29:33, felipeg1 wrote: > On 2012/12/03 19:28:33, glotov wrote: > > BTW, why is this change? > Because we can't yet compile c++0x code for android right now (the sdk doesn't > support yet). > So we can't use the <unordered_set> https://chromiumcodereview.appspot.com/11348354/diff/4002/tools/cygprofile/cy... tools/cygprofile/cygprofile.cc:297: // will generate its own logs that will later have to be merged as usual. Sounds good. My main concern was the system could fork in the context of running Chrome, but as you say it cannot, I am good. On 2012/12/03 20:10:38, digit1 wrote: > On 2012/12/03 19:28:33, glotov wrote: > > Then how does android create chrome sub-processes (renderers, zygotes)? > > We ask the system to create the processes for us. There is no Chromium zygote, > instead, the Dalvik zygote will fork to start any process we need. All this > happens before any Chromium native code runs, hence there is no need for > pthread_atfork(). > > Generally speaking, using fork() is unsupported under Android. While it works, > the corresponding processes can be killed any time by the system, which doesn't > like processes it doesn't have proper accounting for. The browser process, and > up to five renderer processes are all described in the AndroidManifest.xml that > comes with the application. > > Also, using the system allows our render processes to run with additional > sandboxing under JellyBean (4.1) and higher, i.e. these processes run in a > separate UID that is not associated to any installed application. As such, they > have absolutely zero permissions (and can't even access the filesystem). The > system also allows us to send a few file descriptor to setup IPC channels with > them, and that's how things roll... > > It also means we have to reload libchromeview.so in each process, but that's for > another time :)
https://chromiumcodereview.appspot.com/11348354/diff/4002/tools/cygprofile/cy... File tools/cygprofile/cygprofile.cc (right): https://chromiumcodereview.appspot.com/11348354/diff/4002/tools/cygprofile/cy... tools/cygprofile/cygprofile.cc:37: #include "base/hash_tables.h" On 2012/12/04 15:45:46, glotov wrote: > I assume these are equivalent, right? > > On 2012/12/04 12:29:33, felipeg1 wrote: > > On 2012/12/03 19:28:33, glotov wrote: > > > BTW, why is this change? > > Because we can't yet compile c++0x code for android right now (the sdk doesn't > > support yet). > > So we can't use the <unordered_set> > Yes, unordered_set in c++0x is implemented as a hash_set<>
Good, lgtm.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/11348354/4002
Message was sent while issue was closed.
Change committed as 171034 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
