Add CurrentProcessInfo::CreationTime() for Linux

Add CurrentProcessInfo::CreationTime() for Linux

This is used to measure startup performance.

BUG=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216409

[willchan no longer on Chromium, 2013-07-31 05:01:07]

jln/markus: can you advise on whether there's a better way to do this on Linux
than stat() on /proc/self?

https://codereview.chromium.org/21302005/diff/1/base/process/process_info_lin...
File base/process/process_info_linux.cc (right):

https://codereview.chromium.org/21302005/diff/1/base/process/process_info_lin...
base/process/process_info_linux.cc:25: return new
Time(Time::FromTimeT(result.st_ctime));
Uh, what's freeing this memory?

[Markus (顧孟勤), 2013-07-31 06:04:55]

Stat on /proc/self is the best option that I can think of -- of course, that
won't work when you are in a sandbox; and it could get confusing when we keep
fork()'ing things from the zygote.

[jln (very slow on Chromium), 2013-07-31 20:10:54]

On 2013/07/31 06:04:55, Markus (顧孟勤) wrote:
> Stat on /proc/self is the best option that I can think of -- of course, that
> won't work when you are in a sandbox; and it could get confusing when we keep
> fork()'ing things from the zygote.

I think it would be better to open /proc/self/stat and look at the starttime
field.

Just add this field in base/process_util_linux.cc

[Markus (顧孟勤), 2013-07-31 20:17:04]

It probably doesn't really much either way.

But do you honestly believe that the timestamp on /proc/self/. would ever be
out-of-sync from the values in /proc/self/stat? That would be somewhat
disturbing.

And stat()ing /proc/self/. is likely a more light-weight operation than parsing
/proc/self/stat. Whether this makes a measurable difference in practice is
questionable, though.

[jln (very slow on Chromium), 2013-07-31 20:28:26]

On 2013/07/31 20:17:04, Markus (顧孟勤) wrote:
> It probably doesn't really much either way.
> 
> But do you honestly believe that the timestamp on /proc/self/. would ever be
> out-of-sync from the values in /proc/self/stat? That would be somewhat
> disturbing.
> 
> And stat()ing /proc/self/. is likely a more light-weight operation than
parsing
> /proc/self/stat. Whether this makes a measurable difference in practice is
> questionable, though.

It's just a little bit nicer for two reasons:
1. Using documenting APIs is nice, there could also be a higher resolution in
starttime.
2. It's much easier to port it to run inside sandbox. I always had on the back
of my mind to revamp process_util_linux to accept a file descriptor.
The numthread field and a few other fields from "stat" are regularly needed, so
it could make sense for the sandbox to provide an interface to get a fd to stat
that can be passed to the base/ helpers.

[James Simonsen, 2013-08-02 01:31:40]

On 2013/07/31 20:10:54, Julien Tinnes wrote:
> On 2013/07/31 06:04:55, Markus (顧孟勤) wrote:
> > Stat on /proc/self is the best option that I can think of -- of course, that
> > won't work when you are in a sandbox; and it could get confusing when we
keep
> > fork()'ing things from the zygote.
> 
> I think it would be better to open /proc/self/stat and look at the starttime
> field.
> 
> Just add this field in base/process_util_linux.cc

That value is actually the number of ticks since boot time that the process
started.  I had to add a bunch of other stuff to convert it.

https://codereview.chromium.org/21302005/diff/1/base/process/process_info_lin...
File base/process/process_info_linux.cc (right):

https://codereview.chromium.org/21302005/diff/1/base/process/process_info_lin...
base/process/process_info_linux.cc:25: return new
Time(Time::FromTimeT(result.st_ctime));
On 2013/07/31 05:01:07, willchan wrote:
> Uh, what's freeing this memory?

Nobody. It's a singleton.

Are you implying I change the API (for all platforms)?

[willchan no longer on Chromium, 2013-08-02 05:51:52]

LGTM

https://codereview.chromium.org/21302005/diff/1/base/process/process_info_lin...
File base/process/process_info_linux.cc (right):

https://codereview.chromium.org/21302005/diff/1/base/process/process_info_lin...
base/process/process_info_linux.cc:25: return new
Time(Time::FromTimeT(result.st_ctime));
On 2013/08/02 01:31:40, James Simonsen wrote:
> On 2013/07/31 05:01:07, willchan wrote:
> > Uh, what's freeing this memory?
> 
> Nobody. It's a singleton.
> 
> Are you implying I change the API (for all platforms)?

No, I screwed up. I thought ProcessCreationTimeInternal() got called multiple
times and this was leaking. I see it's only called once. This is mostly fine,
although we gotta hope it's not called simultaneously from multiple threads
since that function static isn't locked =/

[James Simonsen, 2013-08-06 17:12:19]

Julien or Markus, do you have any further comments on the /proc/pid/stat
implementation?

[jln (very slow on Chromium), 2013-08-06 18:57:29]

https://codereview.chromium.org/21302005/diff/9001/base/process/process_info_...
File base/process/process_info_linux.cc (right):

https://codereview.chromium.org/21302005/diff/9001/base/process/process_info_...
base/process/process_info_linux.cc:33: static Time* process_creation_time =
ProcessCreationTimeInternal();
This caching seems a bit dangerous on Linux.

We use fork() extensively (see the Zygote) and this would give you the wrong
process creation time.

Could you bind process_creation_time to the current pid (and DCHECK this?). And
explain in the API that this won't work for forked processes?

Also how come you're not using a lock for process_creation_time ?

Thinking about these two pitfalls, I wonder if it wouldn't be better to not
cache this value in this API and let the caller do the caching as appropriate?

[James Simonsen, 2013-08-06 19:09:07]

https://codereview.chromium.org/21302005/diff/9001/base/process/process_info_...
File base/process/process_info_linux.cc (right):

https://codereview.chromium.org/21302005/diff/9001/base/process/process_info_...
base/process/process_info_linux.cc:33: static Time* process_creation_time =
ProcessCreationTimeInternal();
On 2013/08/06 18:57:29, Julien Tinnes wrote:
> This caching seems a bit dangerous on Linux.
> 
> We use fork() extensively (see the Zygote) and this would give you the wrong
> process creation time.
> 
> Could you bind process_creation_time to the current pid (and DCHECK this?).
And
> explain in the API that this won't work for forked processes?
> 
> Also how come you're not using a lock for process_creation_time ?
> 
> Thinking about these two pitfalls, I wonder if it wouldn't be better to not
> cache this value in this API and let the caller do the caching as appropriate?

I'll just remove it. It's done this way because that's how it's done on the
other OSes. They're all messed up in the same way. Will asked the same question
earlier.

[James Simonsen, 2013-08-06 23:08:51]

Okay, I changed the API to not use the singleton any more.

[jln (very slow on Chromium), 2013-08-06 23:44:30]

On 2013/08/06 23:08:51, James Simonsen wrote:
> Okay, I changed the API to not use the singleton any more.

Thanks, lgtm!

(FYI: this won't work within the Linux sandbox and your DCHECK will trigger,
which I think is the right thing to do).

[James Simonsen, 2013-08-07 18:46:35]

John, can I get a rubberstamp for chrome/?

[jam, 2013-08-07 21:18:57]

lgtm

[commit-bot: I haz the power, 2013-08-07 21:24:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/21302005/24001

[commit-bot: I haz the power, 2013-08-08 17:24:28]

Change committed as 216409