Add a histogram for size of the memory used by redirect chain.

Add a histogram to record the size of the memory used by redirect chain.

BUG=310373
TBR=brettw@chromium.org

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

[haitaol1, 2014-04-23 23:09:58]

Brett: please review changes to gurl.h/cc and navigation_entry_impl.cc

Alexei: please review changes to histogram.xml

Donn: FYI

[donnd, 2014-04-24 00:02:55]

On 2014/04/23 23:09:58, haitaol1 wrote:
> Brett: please review changes to gurl.h/cc and navigation_entry_impl.cc
> 
> Alexei: please review changes to histogram.xml
> 
> Donn: FYI

IIRC we're trying to measure the size of the redirects at commit time.  However
it looks to me like this might measure the redirects at other times too. 
Wouldn't it be better to just add the measuring at the SetRedirectChain call in
NavigationControllerImpl::RendererDidNavigate which is done right before the
commit?

[Alexei Svitkine (slow), 2014-04-24 15:26:18]

https://codereview.chromium.org/253333003/diff/10001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/253333003/diff/10001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10582: +<histogram
name="Navigation.RedirectChainSizeInBytes">
Nit: The histogram element supports a "units" tag. So you can put units="bytes"
and then you can simplify the name (i.e. remove the "InBytes" suffix).

https://codereview.chromium.org/253333003/diff/10001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10586: +    Size of the memory used to
store redirect chain in navigation entry.
Please mention when this is logged (periodically? when a certain event happens?)

[haitaol1, 2014-04-25 19:12:56]

On 2014/04/24 00:02:55, Donn wrote:
> On 2014/04/23 23:09:58, haitaol1 wrote:
> > Brett: please review changes to gurl.h/cc and navigation_entry_impl.cc
> > 
> > Alexei: please review changes to histogram.xml
> > 
> > Donn: FYI
> 
> IIRC we're trying to measure the size of the redirects at commit time. 
However
> it looks to me like this might measure the redirects at other times too. 
> Wouldn't it be better to just add the measuring at the SetRedirectChain call
in
> NavigationControllerImpl::RendererDidNavigate which is done right before the
> commit?

done.

[haitaol1, 2014-04-25 19:13:50]

https://codereview.chromium.org/253333003/diff/10001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/253333003/diff/10001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10582: +<histogram
name="Navigation.RedirectChainSizeInBytes">
On 2014/04/24 15:26:19, Alexei Svitkine wrote:
> Nit: The histogram element supports a "units" tag. So you can put
units="bytes"
> and then you can simplify the name (i.e. remove the "InBytes" suffix).

Done.

https://codereview.chromium.org/253333003/diff/10001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10586: +    Size of the memory used to
store redirect chain in navigation entry.
On 2014/04/24 15:26:19, Alexei Svitkine wrote:
> Please mention when this is logged (periodically? when a certain event
happens?)

Done.

[donnd, 2014-04-25 19:17:32]

lgtm

[haitaol1, 2014-04-29 16:44:00]

Friendly ping.

[Alexei Svitkine (slow), 2014-04-29 16:47:25]

LGTM, sorry for the delay

[haitaol1, 2014-05-02 17:24:06]

Adam: can you take a look at changes to gurl*? Basically I added a function to
return memory usage of GURL instance.

[abarth-chromium, 2014-05-02 20:55:13]

I'm not sure I understand the large context for this change.  We used to have
tons of code like this in Blink, but it wasn't well tested and quickly became
stale.  Maybe there's more appetite in the Chromium world to have these sort of
high-maintenance code, but I don't feel qualified to assess that.

In summary, you probably should get brettw to review this CL.

[brettw, 2014-05-02 23:39:31]

I wouldn't bother with this. If you need to estimate size, I think the string
length should be sufficient. There's like 100 bytes extra overhead for the
parsed tracking, but that won't change the order of magnitude size of the
result.

[haitaol1, 2014-05-05 22:00:49]

On 2014/05/02 23:39:31, brettw wrote:
> I wouldn't bother with this. If you need to estimate size, I think the string
> length should be sufficient. There's like 100 bytes extra overhead for the
> parsed tracking, but that won't change the order of magnitude size of the
> result.

Changed to count URL spec length. PTAL.

[Alexei Svitkine (slow), 2014-05-06 14:53:30]

https://codereview.chromium.org/253333003/diff/50001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/253333003/diff/50001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10685: +    Size of the memory used to
store redirect chain in navigation entry. Logged
Hmm, the updated code actually now simply counts the total number of characters
of all URLs in the redirect chain.

Can you update the description to mention that instead? (And units attribute
should probably say "characters").

[haitaol1, 2014-05-07 22:31:23]

https://codereview.chromium.org/253333003/diff/50001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/253333003/diff/50001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10685: +    Size of the memory used to
store redirect chain in navigation entry. Logged
On 2014/05/06 14:53:31, Alexei Svitkine wrote:
> Hmm, the updated code actually now simply counts the total number of
characters
> of all URLs in the redirect chain.
> 
> Can you update the description to mention that instead? (And units attribute
> should probably say "characters").

Done.

[Alexei Svitkine (slow), 2014-05-07 22:38:30]

lgtm

[haitaol1, 2014-05-15 17:10:33]

@brett: your last comment is addressed. Mind taking another look?

[haitaol1, 2014-05-16 17:15:24]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-05-16 17:15:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haitaol@chromium.org/253333003/110001

[commit-bot: I haz the power, 2014-05-16 20:18:57]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-16 20:56:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-16 20:56:29]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[haitaol1, 2014-05-19 17:23:36]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-05-19 17:25:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haitaol@chromium.org/253333003/110001

[commit-bot: I haz the power, 2014-05-19 18:55:36]

Change committed as 271440