Fix stats table on linux. There were two problems:...

Fix stats table on linux.  There were two problems:
  a) We were using the wrong pid for the browser pid
  b) We weren't initializing the StatsTable after running the zygote.

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52962

[Evan Martin, 2010-07-19 20:01:30]

http://codereview.chromium.org/3051003/diff/1/2
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/3051003/diff/1/2#newcode424
chrome/app/chrome_dll_main.cc:424: // leaking shared memory regions on posix
platforms.
We only leak if the stats table is on, right?
Can you put a NOTIMPLEMENTED() here so we don't lose it?

http://codereview.chromium.org/3051003/diff/1/2#newcode581
chrome/app/chrome_dll_main.cc:581: browser_pid =
base::GetParentProcessId(base::GetCurrentProcId());
What about Mac?

http://codereview.chromium.org/3051003/diff/1/2#newcode806
chrome/app/chrome_dll_main.cc:806: InitializeStatsTable(browser_pid,
parsed_command_line);
Can you put a comment here about why you call it again?

[Mike Belshe, 2010-07-19 20:19:23]

http://codereview.chromium.org/3051003/diff/1/2
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/3051003/diff/1/2#newcode424
chrome/app/chrome_dll_main.cc:424: // leaking shared memory regions on posix
platforms.
Done.  I also moved the comment within the if statement so it is more clear that
it applies only to the case where the StatsTable is turned on.

On 2010/07/19 20:01:30, Evan Martin wrote:
> We only leak if the stats table is on, right?
> Can you put a NOTIMPLEMENTED() here so we don't lose it?

http://codereview.chromium.org/3051003/diff/1/2#newcode581
chrome/app/chrome_dll_main.cc:581: browser_pid =
base::GetParentProcessId(base::GetCurrentProcId());
On 2010/07/19 20:01:30, Evan Martin wrote:
> What about Mac?

I was under the belief that both mac and linux follow the same model here?

http://codereview.chromium.org/3051003/diff/1/2#newcode806
chrome/app/chrome_dll_main.cc:806: InitializeStatsTable(browser_pid,
parsed_command_line);
On 2010/07/19 20:01:30, Evan Martin wrote:
> Can you put a comment here about why you call it again?

Done.

[Evan Martin, 2010-07-19 20:22:28]

Unfortunately, Mac is also different.  I included inline the pattern we usually
use to write this -- every posix platform but mac uses the zygote, so we handle
mac first.


LGTM

http://codereview.chromium.org/3051003/diff/5001/6001
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/3051003/diff/5001/6001#newcode425
chrome/app/chrome_dll_main.cc:425: // NOTIMPLEMENTED: we probably need to shut
this down correctly to avoid
I meant NOTIMPLEMENTED(), which prints out a warning.
It probably doesn't matter too much, though.  Definitely either use
NOTIMPLEMENTED(); or leave the comment as a plain TODO.

http://codereview.chromium.org/3051003/diff/5001/6001#newcode580
chrome/app/chrome_dll_main.cc:580: browser_pid =
base::GetParentProcessId(base::GetCurrentProcId());
I'd write this as

#elif defined(OS_MACOSX)
   browser_pid = old code
#else defined(OS_POSIX)
   browser_pid = crazy zygote code, with a comment about how the zygote ruins
everything
#endif

[Evan Martin, 2010-11-22 22:19:22]

http://codereview.chromium.org/3051003/diff/3/chrome/app/chrome_dll_main.cc
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/3051003/diff/3/chrome/app/chrome_dll_main.cc#n...
chrome/app/chrome_dll_main.cc:811: InitializeStatsTable(browser_pid,
parsed_command_line);
Hey, remember this old change?  I am touching this code again for unrelated
reasons and I don't understand this.  We've already initialized the stats table
for the zygote process at L644, and ZygoteMain() will just fork(), so the
subprocess inherits the initialized table here.  Do you remember why you needed
to recreate it?

[Mike Belshe, 2010-11-22 22:30:52]

http://codereview.chromium.org/3051003/diff/3/chrome/app/chrome_dll_main.cc
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/3051003/diff/3/chrome/app/chrome_dll_main.cc#n...
chrome/app/chrome_dll_main.cc:811: InitializeStatsTable(browser_pid,
parsed_command_line);
On 2010/11/22 22:19:22, Evan Martin wrote:
> Hey, remember this old change?  I am touching this code again for unrelated
> reasons and I don't understand this.  We've already initialized the stats
table
> for the zygote process at L644, and ZygoteMain() will just fork(), so the
> subprocess inherits the initialized table here.  Do you remember why you
needed
> to recreate it?

I can't recall exactly what was broken before - but we want the child to open
the shared memory segment separately from the parent.  Line 644 creates the
shared memory in the browser process, line 811 has each child process open it
and use it as a subscriber.  It seems like a fork should be sufficient, I forget
why that was not the case.

[Evan Martin, 2010-11-22 22:32:30]

http://codereview.chromium.org/3051003/diff/3/chrome/app/chrome_dll_main.cc
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/3051003/diff/3/chrome/app/chrome_dll_main.cc#n...
chrome/app/chrome_dll_main.cc:811: InitializeStatsTable(browser_pid,
parsed_command_line);
On 2010/11/22 22:30:52, Mike Belshe wrote:
> On 2010/11/22 22:19:22, Evan Martin wrote:
> > Hey, remember this old change?  I am touching this code again for unrelated
> > reasons and I don't understand this.  We've already initialized the stats
> table
> > for the zygote process at L644, and ZygoteMain() will just fork(), so the
> > subprocess inherits the initialized table here.  Do you remember why you
> needed
> > to recreate it?
> 
> I can't recall exactly what was broken before - but we want the child to open
> the shared memory segment separately from the parent.  Line 644 creates the
> shared memory in the browser process, line 811 has each child process open it
> and use it as a subscriber.  It seems like a fork should be sufficient, I
forget
> why that was not the case.

Can you advise me how I can test whether my refactoring breaks this?  I run with
--enable-stats-table, and then check that [fill me in].

[mbelshe, 2010-11-22 22:36:52]

On Mon, Nov 22, 2010 at 2:32 PM, <evan@chromium.org> wrote:

>
> http://codereview.chromium.org/3051003/diff/3/chrome/app/chrome_dll_main.cc
> File chrome/app/chrome_dll_main.cc (right):
>
>
>
http://codereview.chromium.org/3051003/diff/3/chrome/app/chrome_dll_main.cc#n...
> chrome/app/chrome_dll_main.cc:811: InitializeStatsTable(browser_pid,
> parsed_command_line);
> On 2010/11/22 22:30:52, Mike Belshe wrote:
>
>> On 2010/11/22 22:19:22, Evan Martin wrote:
>> > Hey, remember this old change?  I am touching this code again for
>>
> unrelated
>
>> > reasons and I don't understand this.  We've already initialized the
>>
> stats
>
>> table
>> > for the zygote process at L644, and ZygoteMain() will just fork(),
>>
> so the
>
>> > subprocess inherits the initialized table here.  Do you remember why
>>
> you
>
>> needed
>> > to recreate it?
>>
>
>  I can't recall exactly what was broken before - but we want the child
>>
> to open
>
>> the shared memory segment separately from the parent.  Line 644
>>
> creates the
>
>> shared memory in the browser process, line 811 has each child process
>>
> open it
>
>> and use it as a subscriber.  It seems like a fork should be
>>
> sufficient, I forget
>
>> why that was not the case.
>>
>
> Can you advise me how I can test whether my refactoring breaks this?  I
> run with --enable-stats-table, and then check that [fill me in].



Go to the URL about:stats

if you see lots of counters, you're working.

Mike


>
>
> http://codereview.chromium.org/3051003/
>