commit 5a166e371c2520a5784d05117e8d2115b78dff4a
author Dan McArdle <dmcardle@chromium.org> Wed Aug 11 00:55:10 2021
committer Chromium LUCI CQ <chromium-scoped@luci-project-accounts.iam.gserviceaccount.com> Wed Aug 11 00:55:10 2021
tree ee59df19d244fa4bef657a94adbbfa19fe043815
parent 2b6d1d2109a9b67fbc81314085ea51cb845cdb3a

Reland "Optimize VlogInfo::GetVlogLevel"

This is a reland of 290a1c1f74bb9943d3673eeb6e6d2feb1b02577a

Prior to the reverted change, no synchronization was needed because we
only read VlogInfo::vmodule_levels_; it was never modified outside of
the constructor.

For this reland, I've added a base::Lock to guard the field. This is
necessary because one of the optimizations involves swapping the most
desirable values of vmodule_levels_ towards the front.

At this point in time, I was not quite able to reproduce the performance
improvement described in the original message.

Recomputing the stats over four runs on an official build, using the
same GN args:
- Before:             the perftest averages 10 gets/us (631ms total)
- With reverted code: the perftest averages 10 gets/us (551.25ms total)
- With base::Lock:    the perftest averages 11 gets/us (564.5ms total)

Percentage-wise, the before-and-after improvement now is 10.5%. Included
in that, adding the base::Lock was only a 2% penalty.

Original change's description:
> Optimize VlogInfo::GetVlogLevel
>
> On ChromeOS-Wide Profiling, I noticed that VlogInfo::GetVlogLevel takes
> up 0.78% of running time, which seems significant.
>
> This CL adds a perftest and the changes described below.
>
> 1. I believe that calls to GetVlogLevel have high locality. That is, the
>    function is likely to see calls from the same file near each other.
>    We can maintain the vector of patterns such that the
>    highest-probability patterns are near the front.
>
> 2. There's a bunch of places where we used references to StringPiece
>    instead of just plain values. This is an easy fix and it's
>    recommended by StringPiece's documentation.
>
> 3. The GetModule() helper function, which removes the file extension,
>    "-inl" suffix, and the path leading up to the file name, can be
>    reorganized to do less work on each of its right-to-left scans of the
>    input string. Specifically, we want to remove the file extension and
>    "-inl" suffix *before* scanning right-to-left for the last forward or
>    backward slash.
>
> Hastily-computed results on my Linux workstation, averaged over two
> runs on an official build:
>   - Before: the perftest averages 10 gets/us (609ms total).
>   - After: the perftest averages 13 gets/us (476ms total).
>
> That's an improvement of 22%! (Calculated as 1 - 476/609.)
>
> Aside: on my local builds with GN args "is_debug = false", I observed
> that redundant occurrences of CHECK_LT were not being optimized, but
> when converted to plain CHECK, they were correctly optimized. This
> stopped happening when I switched to an official build. Specifically,
> the bounds check in base::StringPiece::at was not being eliminated, even
> though MatchVlogPattern does its own bounds checking.
>
> GN args used:
>     is_debug = false
>     is_component_build = false
>     is_official_build = true
>     blink_symbol_level = 0
>     symbol_level = 0
>
> Change-Id: I226b4ea67fc46a1b075f02b7e47c0be073a40301
> Bug: 489441
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3046418
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Commit-Queue: Dan McArdle <dmcardle@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#906903}

Bug: 489441, 1238262
Change-Id: Iefd94c171db02e91ad3a01d5f49531ce1711ebdb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3086662
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Dan McArdle <dmcardle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#910601}