Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2545603002:
Small fixes for url_index_private_data.cc (Closed)
DescriptionSmall fixes for url_index_private_data.cc
This modernises calls to erase so that they would work with any kind of
maps, not only standard ones. Also adds a useful histogram.
This patch is a result of a discussion here:
https://codereview.chromium.org/2333253002/
Committed: https://crrev.com/4f95d672cd17f6c886e985f06593f5b719e7f9d3
Cr-Commit-Position: refs/heads/master@{#435761}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Review, round 1 #
Messages
Total messages: 21 (9 generated)
Description was changed from ========== Small fixes for url_index_private_data.cc This modernises calls to erase so that they would work with any kind of maps, not only standard ones. Also adds a useful histogram. This patch is a result of a here: https://codereview.chromium.org/2333253002/ ========== to ========== Small fixes for url_index_private_data.cc This modernises calls to erase so that they would work with any kind of maps, not only standard ones. Also adds a useful histogram. This patch is a result of a here: https://codereview.chromium.org/2333253002/ ==========
dyaroshev@yandex-team.ru changed reviewers: + mpearson@google.com, pkasting@chromium.org
Description was changed from ========== Small fixes for url_index_private_data.cc This modernises calls to erase so that they would work with any kind of maps, not only standard ones. Also adds a useful histogram. This patch is a result of a here: https://codereview.chromium.org/2333253002/ ========== to ========== Small fixes for url_index_private_data.cc This modernises calls to erase so that they would work with any kind of maps, not only standard ones. Also adds a useful histogram. This patch is a result of a discussion here: https://codereview.chromium.org/2333253002/ ==========
LGTM on the code change assuming this histogram is actually needed. You'll need a relevant OWNER to approve the histogram change. https://codereview.chromium.org/2545603002/diff/1/components/omnibox/browser/... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2545603002/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.cc:500: SCOPED_UMA_HISTOGRAM_TIMER("Omnibox.HistoryIDSetFromWords"); Double-checking: the existing omnibox provider time histograms are insufficiently granular to measure HQP performance wins in the field? (I want to make sure you know about these histograms before adding this, since my own knowledge of them is a little too sketchy to know whether this is good or not.) https://codereview.chromium.org/2545603002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2545603002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:40756: + HistoryQuick provider works, which could happen multiple times on each "works" is kind of vague. It seems like a more precise description might be good.
On 2016/11/30 21:39:50, Peter Kasting wrote: > LGTM on the code change assuming this histogram is actually needed. You'll need > a relevant OWNER to approve the histogram change. > > https://codereview.chromium.org/2545603002/diff/1/components/omnibox/browser/... > File components/omnibox/browser/url_index_private_data.cc (right): > > https://codereview.chromium.org/2545603002/diff/1/components/omnibox/browser/... > components/omnibox/browser/url_index_private_data.cc:500: > SCOPED_UMA_HISTOGRAM_TIMER("Omnibox.HistoryIDSetFromWords"); > Double-checking: the existing omnibox provider time histograms are > insufficiently granular to measure HQP performance wins in the field? (I want > to make sure you know about these histograms before adding this, since my own > knowledge of them is a little too sketchy to know whether this is good or not.) > No, they are more than sufficient. But this is the part that we optimise. So if you see a big number on this histogram, that means that HQP performed badly because of sets manipulation, not because of string checks or something else. > https://codereview.chromium.org/2545603002/diff/1/tools/metrics/histograms/hi... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2545603002/diff/1/tools/metrics/histograms/hi... > tools/metrics/histograms/histograms.xml:40756: + HistoryQuick provider works, > which could happen multiple times on each > "works" is kind of vague. It seems like a more precise description might be > good. Runs?
On 2016/11/30 21:43:05, dyaroshev wrote: > On 2016/11/30 21:39:50, Peter Kasting wrote: > > LGTM on the code change assuming this histogram is actually needed. You'll > need > > a relevant OWNER to approve the histogram change. > > > > > https://codereview.chromium.org/2545603002/diff/1/components/omnibox/browser/... > > File components/omnibox/browser/url_index_private_data.cc (right): > > > > > https://codereview.chromium.org/2545603002/diff/1/components/omnibox/browser/... > > components/omnibox/browser/url_index_private_data.cc:500: > > SCOPED_UMA_HISTOGRAM_TIMER("Omnibox.HistoryIDSetFromWords"); > > Double-checking: the existing omnibox provider time histograms are > > insufficiently granular to measure HQP performance wins in the field? (I want > > to make sure you know about these histograms before adding this, since my own > > knowledge of them is a little too sketchy to know whether this is good or > not.) > > > > No, they are more than sufficient. But this is the part that we optimise. So if > you see a big number on this histogram, that means that HQP performed badly > because of sets manipulation, not because of string checks or something else. Do such other things take very much time today, or are likely to in the future? I'm not strongly opposed to adding this, but I don't know whether we need it, if we can figure out positive and negative impact to this function with good confidence from the performance tests + the existing histograms. > > > https://codereview.chromium.org/2545603002/diff/1/tools/metrics/histograms/hi... > > File tools/metrics/histograms/histograms.xml (right): > > > > > https://codereview.chromium.org/2545603002/diff/1/tools/metrics/histograms/hi... > > tools/metrics/histograms/histograms.xml:40756: + HistoryQuick provider > works, > > which could happen multiple times on each > > "works" is kind of vague. It seems like a more precise description might be > > good. > > Runs? That's not actually more precise :). Why can this happen multiple times per keystroke -- is it once per OmniboxController::StartAutocomplete(), but that's getting called multiple times due to http://crbug.com/669727 ? Or is it something else?
On 2016/11/30 21:50:13, Peter Kasting wrote: > On 2016/11/30 21:43:05, dyaroshev wrote: > > On 2016/11/30 21:39:50, Peter Kasting wrote: > > > LGTM on the code change assuming this histogram is actually needed. You'll > > need > > > a relevant OWNER to approve the histogram change. > > > > > > > > > https://codereview.chromium.org/2545603002/diff/1/components/omnibox/browser/... > > > File components/omnibox/browser/url_index_private_data.cc (right): > > > > > > > > > https://codereview.chromium.org/2545603002/diff/1/components/omnibox/browser/... > > > components/omnibox/browser/url_index_private_data.cc:500: > > > SCOPED_UMA_HISTOGRAM_TIMER("Omnibox.HistoryIDSetFromWords"); > > > Double-checking: the existing omnibox provider time histograms are > > > insufficiently granular to measure HQP performance wins in the field? (I > want > > > to make sure you know about these histograms before adding this, since my > own > > > knowledge of them is a little too sketchy to know whether this is good or > > not.) > > > > > > > No, they are more than sufficient. But this is the part that we optimise. So > if > > you see a big number on this histogram, that means that HQP performed badly > > because of sets manipulation, not because of string checks or something else. > > Do such other things take very much time today, or are likely to in the future? > I'm not strongly opposed to adding this, but I don't know whether we need it, if > we can figure out positive and negative impact to this function with good > confidence from the performance tests + the existing histograms. > HQP is very vage - it does lot's of staff. I'm not sure if it's easy to tell what went wrong and why is everything slow. This just measures maps. I think that could be a good to have in case something would went wrong with flat containers or otherwise. We still didn't add created perftest to regular runs, so I don't know if you'll get data right away. Anyways, if you don't want it, I don't insist). > > > > > > https://codereview.chromium.org/2545603002/diff/1/tools/metrics/histograms/hi... > > > File tools/metrics/histograms/histograms.xml (right): > > > > > > > > > https://codereview.chromium.org/2545603002/diff/1/tools/metrics/histograms/hi... > > > tools/metrics/histograms/histograms.xml:40756: + HistoryQuick provider > > works, > > > which could happen multiple times on each > > > "works" is kind of vague. It seems like a more precise description might be > > > good. > > > > Runs? > > That's not actually more precise :). Why can this happen multiple times per > keystroke -- is it once per OmniboxController::StartAutocomplete(), but that's > getting called multiple times due to http://crbug.com/669727 ? Or is it > something else? Seems like this is the bug. When we call Classify we run HQP. Sometimes we run classify a few times.
On 2016/11/30 22:04:22, dyaroshev wrote: > On 2016/11/30 21:50:13, Peter Kasting wrote: > > Do such other things take very much time today, or are likely to in the > future? > > I'm not strongly opposed to adding this, but I don't know whether we need it, > if > > we can figure out positive and negative impact to this function with good > > confidence from the performance tests + the existing histograms. > > > > HQP is very vage - it does lot's of staff. I'm not sure if it's easy to tell > what went wrong and why is everything slow. > This just measures maps. I think that could be a good to have in case something > would went wrong with flat containers or otherwise. > > We still didn't add created perftest to regular runs, so I don't know if you'll > get data right away. > > Anyways, if you don't want it, I don't insist). I'll let Mark make the call. > > That's not actually more precise :). Why can this happen multiple times per > > keystroke -- is it once per OmniboxController::StartAutocomplete(), but that's > > getting called multiple times due to http://crbug.com/669727 ? Or is it > > something else? > > Seems like this is the bug. When we call Classify we run HQP. Sometimes we run > classify a few times. Can you get callstacks to make sure the bug covers the cases where this can happen? Running it "a few times" sounds like there may be more bugs than the known case covered there.
mpearson@chromium.org changed reviewers: + mpearson@chromium.org
Some minor comments. If you think this will be useful to monitor in an ongoing basis as you make improvements, I'm not opposed. I am a metrics owner so once you get a lgtm from me, you're good to go. --mark https://codereview.chromium.org/2545603002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2545603002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:40752: +<histogram name="Omnibox.HistoryIDSetFromWordsForked" units="ms"> This name does not match the name you used in the code. Also, I suggest a name that makes it obvious that this only applies to the HistoryQuick part of the omnibox. E.g., Omnibox.HistoryQuickHistoryID... or Omnibox.HQPHistoryID... https://codereview.chromium.org/2545603002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:40756: + HistoryQuick provider works, which could happen multiple times on each I suggest a rewording (with extra detail), something like the following: Times URLIndexPrivateData::HistoryIDSetFromWords(), which is called by the omnibox's HistoryQuick provider. URLIndexPrivateData::HistoryIDSetFromWords() can be called multiple times per keystroke due to, for example, the cursor being in the middle of the input string or SearchProvider's calls to Classify().
On 2016/11/30 22:57:59, Mark P wrote: > Some minor comments. If you think this will be useful to monitor in an ongoing > basis as you make improvements, I'm not opposed. I am a metrics owner so once > you get a lgtm from me, you're good to go. > > --mark > > https://codereview.chromium.org/2545603002/diff/1/tools/metrics/histograms/hi... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2545603002/diff/1/tools/metrics/histograms/hi... > tools/metrics/histograms/histograms.xml:40752: +<histogram > name="Omnibox.HistoryIDSetFromWordsForked" units="ms"> > This name does not match the name you used in the code. > > Also, I suggest a name that makes it obvious that this only applies to the > HistoryQuick part of the omnibox. E.g., Omnibox.HistoryQuickHistoryID... or > Omnibox.HQPHistoryID... > > https://codereview.chromium.org/2545603002/diff/1/tools/metrics/histograms/hi... > tools/metrics/histograms/histograms.xml:40756: + HistoryQuick provider works, > which could happen multiple times on each > I suggest a rewording (with extra detail), something like the following: > Times URLIndexPrivateData::HistoryIDSetFromWords(), which is called by the > omnibox's HistoryQuick provider. URLIndexPrivateData::HistoryIDSetFromWords() > can be called multiple times per keystroke due to, for example, the cursor being > in the middle of the input string or SearchProvider's calls to Classify(). I think I fixed everything, just in case, please recheck.
On 2016/11/30 22:23:46, Peter Kasting wrote: > > > That's not actually more precise :). Why can this happen multiple times per > > > keystroke -- is it once per OmniboxController::StartAutocomplete(), but > that's > > > getting called multiple times due to http://crbug.com/669727 ? Or is it > > > something else? > > > > Seems like this is the bug. When we call Classify we run HQP. Sometimes we run > > classify a few times. > > Can you get callstacks to make sure the bug covers the cases where this can > happen? Running it "a few times" sounds like there may be more bugs than the > known case covered there. First try didn't show any results. If I'll find anything, I'll add a comment to that task.
lgtm
The CQ bit was checked by dyaroshev@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2545603002/#ps20001 (title: "Review, round 1")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480629493397680,
"parent_rev": "1bbcd80f6ded81706ee79b302dcadc84f6288c46", "commit_rev":
"0088f62e20b786096034a2dd342488358a7da255"}
Message was sent while issue was closed.
Description was changed from ========== Small fixes for url_index_private_data.cc This modernises calls to erase so that they would work with any kind of maps, not only standard ones. Also adds a useful histogram. This patch is a result of a discussion here: https://codereview.chromium.org/2333253002/ ========== to ========== Small fixes for url_index_private_data.cc This modernises calls to erase so that they would work with any kind of maps, not only standard ones. Also adds a useful histogram. This patch is a result of a discussion here: https://codereview.chromium.org/2333253002/ ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Small fixes for url_index_private_data.cc This modernises calls to erase so that they would work with any kind of maps, not only standard ones. Also adds a useful histogram. This patch is a result of a discussion here: https://codereview.chromium.org/2333253002/ ========== to ========== Small fixes for url_index_private_data.cc This modernises calls to erase so that they would work with any kind of maps, not only standard ones. Also adds a useful histogram. This patch is a result of a discussion here: https://codereview.chromium.org/2333253002/ Committed: https://crrev.com/4f95d672cd17f6c886e985f06593f5b719e7f9d3 Cr-Commit-Position: refs/heads/master@{#435761} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4f95d672cd17f6c886e985f06593f5b719e7f9d3 Cr-Commit-Position: refs/heads/master@{#435761} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
