Chromium Code Reviews
Help | Chromium Project | Sign in
(68)

Issue 3051003: Fix stats table on linux. There were two problems:... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 10 months ago by Mike Belshe
Modified:
3 years, 11 months ago
Reviewers:
mbelshe, Evan Martin
CC:
chromium-reviews
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 6

Patch Set 2 : '' #

Total comments: 2

Patch Set 3 : '' #

Patch Set 4 : '' #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -17 lines) Patch
M chrome/app/chrome_dll_main.cc View 1 2 3 4 chunks +31 lines, -17 lines 3 comments Download
Commit: CQ not working?

Messages

Total messages: 8 (0 generated)
Mike Belshe
4 years, 10 months ago (2010-07-19 19:58:04 UTC) #1
Evan Martin
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 ...
4 years, 10 months ago (2010-07-19 20:01:30 UTC) #2
Mike Belshe
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. ...
4 years, 10 months ago (2010-07-19 20:19:23 UTC) #3
Evan Martin
Unfortunately, Mac is also different. I included inline the pattern we usually use to write ...
4 years, 10 months ago (2010-07-19 20:22:28 UTC) #4
Evan Martin
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#newcode811 chrome/app/chrome_dll_main.cc:811: InitializeStatsTable(browser_pid, parsed_command_line); Hey, remember this old change? I am ...
4 years, 6 months ago (2010-11-22 22:19:22 UTC) #5
Mike Belshe
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#newcode811 chrome/app/chrome_dll_main.cc:811: InitializeStatsTable(browser_pid, parsed_command_line); On 2010/11/22 22:19:22, Evan Martin wrote: > ...
4 years, 6 months ago (2010-11-22 22:30:52 UTC) #6
Evan Martin
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#newcode811 chrome/app/chrome_dll_main.cc:811: InitializeStatsTable(browser_pid, parsed_command_line); On 2010/11/22 22:30:52, Mike Belshe wrote: > ...
4 years, 6 months ago (2010-11-22 22:32:30 UTC) #7
mbelshe
4 years, 6 months ago (2010-11-22 22:36:52 UTC) #8
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/
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld ec887be