Closed
Bug 1120409
Opened 9 years ago
Closed 7 years ago
nsITelemetry.histogramSnapshots should not return keyed histograms
Categories
(Toolkit :: Telemetry, defect, P3)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: Dexter, Assigned: gmoore, Mentored)
Details
(Whiteboard: [lang=c++])
Attachments
(3 files, 4 obsolete files)
Keyed histograms are displayed in the "Histograms" section of about:telemetry as well as in the "Keyed Histograms" section (e.g. "SEARCH_COUNTS#google.searchbar").
Comment 1•9 years ago
|
||
nsITelemetry.histogramSnapshots returns all histograms. Instead it should filter out keyed histograms. Note that this is not an issue with the actual outgoing Telemetry pings, as that implementation happens to filter out keyed histograms.
Mentor: gfritzsche
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [lang=c++]
Comment 2•9 years ago
|
||
The histogramSnapshots implementation is here: https://hg.mozilla.org/mozilla-central/annotate/fa91879c8428/toolkit/components/telemetry/Telemetry.cpp#l2117 This should filter out keyed histograms. IdentifyCorruptHistograms() shows how to get the enum id. With that we can check |gHistograms[id].keyed| and filter them out.
Comment 3•9 years ago
|
||
Vladan, do you think the keyed histograms should affect the global corruption-statistics for histograms? I.e., should we filter out keyed histograms in GetHistogramSnapshots() before or after IdentifyCorruptHistograms()?
Flags: needinfo?(vdjeric)
Comment 4•9 years ago
|
||
Does IdentifyCorruptHistograms even work correctly for keyed-histograms right now? It uses gCorruptHistograms[Telemetry::HistogramCount] to store corruption state per histogram, but HistogramCount is the static number of histograms defined in Histograms.json.. How does it deal with a variable number of keyed histograms? I think this should be looked into. To answer your question, I think we should filter on keyed/non-keyed in the "Get..Snapshots" functions first and then call IdentifyCorruptHistograms on the remaining histograms. That way each "Get..Snapshots" function would check that only the histograms it's about to return aren't corrupted.
Flags: needinfo?(vdjeric)
Comment 5•9 years ago
|
||
(In reply to Vladan Djeric (:vladan) from comment #4) > Does IdentifyCorruptHistograms even work correctly for keyed-histograms > right now? Actually it doesn't do anything with them at all. For a keyed histogram with name "A" and used keys "B" and "C" we have histograms named "A#B" and "A#C". We don't find an id for them here: https://hg.mozilla.org/mozilla-central/annotate/38e4719e71af/toolkit/components/telemetry/Telemetry.cpp#l1944 ... so we just skip them in IdentifyCorrupHistograms(). ni?me to setup proper mentored-bug advice.
Flags: needinfo?(gfritzsche)
Comment 6•9 years ago
|
||
Per bug 1122480 we don't show the keyed histograms anymore in the plain histogram section in about:telemetry (we now show the data directly from pings).
Summary: Keyed histograms should only be shown in keyed histograms section in about:telemetry → nsITelemetry.histogramSnapshots should not return keyed histograms
Comment 7•9 years ago
|
||
Georg, I'm interested in working on this bug. Can you please give me more info on where to get started?
Comment 8•9 years ago
|
||
(In reply to Ryan Nath from comment #7) > I'm interested in working on this bug. Can you please give me more info on > where to get started? Hi Ryan, sure! We'll want to skip keyed histograms for reflection around here: https://hg.mozilla.org/mozilla-central/annotate/ac277e615f8f/toolkit/components/telemetry/Telemetry.cpp#l2302 There is a check for whether a histogram is keyed here: https://hg.mozilla.org/mozilla-central/annotate/ac277e615f8f/toolkit/components/telemetry/Telemetry.cpp#l2272 To do that check we need to get the |TelemetryHistogram| for the |Histogram*|, there is some examples of how to get that around in the tree: You need to use TelemetryImpl::GetHistogramEnumId() with the histograms name to get the id, with that you can do the check above. about:telemetry doesn't have that issue anymore, but for quick checking you can use the Scratchpad. * Make sure chrome devtools are enabled * tools -> web developer -> scratchpad * switch the scratchpads environment to "browser" Use something like the following and hit "inspect": > let telemetry = Services.telemetry; > let keyed = telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_COUNT"); > keyed.add("a", 1); > Services.telemetry.histogramSnapshots Filtering the properties you'll see TELEMETRY_TEST_KEYED_COUNT before the changes. For testing you'll want: * run the most relevant test with: mach xpcshell-test toolkit/components/telemetry/tests/unit/test_nsITelemetry.js * extend that test with a new test function around here: https://hg.mozilla.org/mozilla-central/annotate/ac277e615f8f/toolkit/components/telemetry/tests/unit/test_nsITelemetry.js#l874 * that function should make sure a keyed histogram was created like the snippet above * and then check that the keyed histogram is not in the histogramSnapshots After things work make sure this didn't break other Telemetry behavior by running: mach xpcshell-test toolkit/components/telemetry/tests/unit/ ... let me know if anything is unclear.
Flags: needinfo?(gfritzsche)
Comment 10•9 years ago
|
||
Nothing happened here since comment 8. Are you interested in taking on this bug?
Flags: needinfo?(gfritzsche)
Comment 11•8 years ago
|
||
Hello, I would like to work on this bug
Comment 12•8 years ago
|
||
I think this is a bit involved for a first bug, i'd recommend filtering e.g. here for "simple" / "good first" bugs: http://www.joshmatthews.net/bugsahoy/?unowned=1&simple=1
Comment 14•8 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] [away Jun 24 - Jul 3] from comment #8) > We'll want to skip keyed histograms for reflection around here: > https://hg.mozilla.org/mozilla-central/annotate/ac277e615f8f/toolkit/ > components/telemetry/Telemetry.cpp#l2302 > > There is a check for whether a histogram is keyed here: > https://hg.mozilla.org/mozilla-central/annotate/ac277e615f8f/toolkit/ > components/telemetry/Telemetry.cpp#l2272 > > To do that check we need to get the |TelemetryHistogram| for the > |Histogram*|, there is some examples of how to get that around in the tree: > You need to use TelemetryImpl::GetHistogramEnumId() with the histograms name > to get the id, with that you can do the check above. > > about:telemetry doesn't have that issue anymore, but for quick checking you > can use the Scratchpad. > * Make sure chrome devtools are enabled > * tools -> web developer -> scratchpad > * switch the scratchpads environment to "browser" > > Use something like the following and hit "inspect": > > let telemetry = Services.telemetry; > > let keyed = telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_COUNT"); > > keyed.add("a", 1); > > Services.telemetry.histogramSnapshots > > Filtering the properties you'll see TELEMETRY_TEST_KEYED_COUNT before the > changes. This is not correct, this shows up as "TELEMETRY_TEST_KEYED_COUNT#a" (because we store each entry in a keyed histogram in the format "HISTOGRAM_NAME#key"). Note that the serialization functions linked to in comment 8 moved from Telemetry.cpp to TelemetryHistogram.cpp, otherwise they should be fundamentally the same.
Assignee | ||
Comment 16•7 years ago
|
||
Hi, I am interested in working on this bug if no one else is and this is still an issue. It looks like there's already enough information to get started, but I just wanted to check if there was anything else I should know. Also, I don't have much experience with JavaScript, so I wanted to check if that would be a problem. Let me know, thanks.
Flags: needinfo?(gfritzsche)
Comment 17•7 years ago
|
||
Hi Gregory, no-one else is working on this currently. This bug doesn't require special JavaScript or C++ knowledge: the changes required here are rather contained. Comment 8 & comment 14 should have the relevant information. The only thing that changed really is that the code was moved to TelemetryHistogram.cpp: https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/toolkit/components/telemetry/TelemetryHistogram.cpp#2320 I would recommend to manually confirm the problem first in the browser before getting started.
Flags: needinfo?(gfritzsche)
Updated•7 years ago
|
Mentor: alessio.placitelli
Comment 18•7 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #8) > * run the most relevant test with: mach xpcshell-test > toolkit/components/telemetry/tests/unit/test_nsITelemetry.js > * extend that test with a new test function around here: > https://hg.mozilla.org/mozilla-central/annotate/ac277e615f8f/toolkit/ > components/telemetry/tests/unit/test_nsITelemetry.js#l874 Also, this test was renamed since then: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
Assignee | ||
Comment 19•7 years ago
|
||
Okay cool, thanks for replying and for the information. (In reply to Georg Fritzsche [:gfritzsche] from comment #17) > I would recommend to manually confirm the problem first in the browser > before getting started. > Filtering the properties you'll see TELEMETRY_TEST_KEYED_COUNT before the > changes. I'm not exactly sure what you mean by see "TELEMETRY_TEST_KEYED_COUNT" (or "TELEMETRY_TEST_KEYED_COUNT#a" rather, from comment 14) before the changes. Do you just mean before we fix the problem? I can run the JavaScript code and I get the output shown in the screenshot I attached. I do see the TELEMETRY_TEST_KEYED_COUNT#a property, so I'm guessing the problem is still there, but I just wanted to make sure I was checking for it correctly. Let me know, thanks.
Flags: needinfo?(gfritzsche)
Comment 20•7 years ago
|
||
(In reply to Gregory Moore [:gmoore] from comment #19) > Created attachment 8838239 [details] > Screenshot of ScratchPad output. > > Okay cool, thanks for replying and for the information. > > (In reply to Georg Fritzsche [:gfritzsche] from comment #17) > > I would recommend to manually confirm the problem first in the browser > > before getting started. > > > Filtering the properties you'll see TELEMETRY_TEST_KEYED_COUNT before the > > changes. > I'm not exactly sure what you mean by see "TELEMETRY_TEST_KEYED_COUNT" (or > "TELEMETRY_TEST_KEYED_COUNT#a" rather, from comment 14) before the changes. > Do you just mean before we fix the problem? I can run the JavaScript code > and I get the output shown in the screenshot I attached. I do see the > TELEMETRY_TEST_KEYED_COUNT#a property, so I'm guessing the problem is still > there, but I just wanted to make sure I was checking for it correctly. Let > me know, thanks. Exactly, we should still see this before we the problem. It means that the problem still exists.
Flags: needinfo?(gfritzsche)
Assignee | ||
Comment 21•7 years ago
|
||
Okay, cool, thanks. I finished an initial version of the patch, and it seems to fix the problem. test_TelemetryHistograms.js as well as all of the other tests in toolkit/components/telemetry/tests/unit/ are currently passing, and I no longer see the keyed histograms in ScratchPad.
It seemed like at first I couldn't ever get the "TELEMETRY_TEST_KEYED_COUNT#a" property to be returned by Services.telemetry.histogramSnapshots on my local build. I added the line
> export MOZ_TELEMETRY_REPORTING=1
to my mozconfig file though, and that seemed to make it show up. Not sure what that means for testing though (do we need to make sure telemetry is enabled before running the tests?).
Also, I used Telemetry instead of Services.telemetry in the JavaScript test (it seems to give the same results). Let me know if this isn't correct, though.
Also, in the case that getting the histogram ID fails, we just continue in the loop. I'm not sure if we should be returning the nsresult (rv) or doing something else instead though.
Let me know if there are any other problems or things to change. Thanks.
Flags: needinfo?(gfritzsche)
Comment 22•7 years ago
|
||
Comment on attachment 8840165 [details] [diff] [review] Initial version of patch. Review of attachment 8840165 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Gregory, this looks good already and only needs small changes. Sorry it took a while to review, somehow it dropped off my radar. I think this should not fail in any test builds, a lot of other test code depends on the snapshots to work. We can make sure of that by pushing it to our "try" test servers before landing the patch. ::: toolkit/components/telemetry/TelemetryHistogram.cpp @@ +2340,5 @@ > + mozilla::Telemetry::ID id; > + nsresult rv = internal_GetHistogramEnumId(h->histogram_name().c_str(), &id); > + if (NS_FAILED(rv)) { > + continue; > + } else if (gHistograms[id].keyed) { Lets make this a bit shorter and log warnings too: if (NS_WARN_IF(NS_FAILED(rv)) || gHistograms[id].keyed) { ... ::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js @@ +757,5 @@ > "Keyed histogram add should not record when recording is disabled"); > }); > > +add_task(function* test_histogram_snapshots() { > + let keyed = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_COUNT"); The indentation should be 2 spaces. @@ +760,5 @@ > +add_task(function* test_histogram_snapshots() { > + let keyed = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_COUNT"); > + keyed.add("a", 1); > + > + //Check that keyed hisograms are not returned Nit: Space before "Check"
Attachment #8840165 -
Flags: feedback+
Assignee | ||
Comment 23•7 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #22) > Thanks Gregory, this looks good already and only needs small changes. > Sorry it took a while to review, somehow it dropped off my radar. No problem. I updated the patch with the changes you mentioned. Let me know if there are more problems or things to change. Thanks. Also, when I ran the other Telemetry tests on my local build (the tests in toolkit/components/telemetry/tests/unit/), one of them kept timing out. Not sure why it was doing that, but hopefully it will work on the try server.
Attachment #8840165 -
Attachment is obsolete: true
Attachment #8844681 -
Flags: review?(gfritzsche)
Updated•7 years ago
|
Assignee: nobody → olucafont6
Flags: needinfo?(gfritzsche)
Comment 24•7 years ago
|
||
Comment on attachment 8844681 [details] [diff] [review] Made some small changes and fixed some formatting errors. Review of attachment 8844681 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, thanks. There is one small typo below. Also, lets shorten the commit message a little to focus on the results? E.g.: "Bug xxx - nsITelemetry.histogramSnapshots will no longer return keyed histograms. r=gfritzsche" The implementation details can be seen from the code changes here. The timeout was probably with test_ThreadHangStats.js? That one is problematic and needs work, it takes a long time to run. I pushed the changes to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43bfbe44d997b89bca71ec7435eac4ebc1a9893a ::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js @@ +760,5 @@ > +add_task(function* test_histogramSnapshots() { > + let keyed = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_COUNT"); > + keyed.add("a", 1); > + > + // Check that keyed hisograms are not returned Typo, i missed that before: "keyed histograms"
Attachment #8844681 -
Flags: review?(gfritzsche) → review+
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 25•7 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #24) > Typo, i missed that before: "keyed histograms" Oh whoops, I missed that as well. Thanks for catching that. > The timeout was probably with test_ThreadHangStats.js? That one is > problematic and needs work, it takes a long time to run. I tried it again and (that time at least) it was test_TelemetrySession.js. Not sure why it's timing out... I updated the patch. Thanks for all of the help with this bug, I appreciate it.
Attachment #8844681 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8845187 -
Flags: review?(gfritzsche)
Comment 26•7 years ago
|
||
Comment on attachment 8845187 [details] [diff] [review] Fixed a spelling error and shortened the commit message. Review of attachment 8845187 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, thanks!
Attachment #8845187 -
Flags: review?(gfritzsche) → review+
Updated•7 years ago
|
Keywords: checkin-needed
Comment 27•7 years ago
|
||
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/mozilla-inbound/rev/5733e0b0e096 nsITelemetry.histogramSnapshots will no longer return keyed histograms. r=gfritzsche
Keywords: checkin-needed
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5733e0b0e096
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 29•7 years ago
|
||
backed this out from central, iris told me this made https://treeherder.mozilla.org/logviewer.html#?job_id=85239109&repo=mozilla-inbound failing more frequently
Flags: needinfo?(olucafont6)
Updated•7 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 30•7 years ago
|
||
Backout by cbook@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/ca4ae502156e Backed out changeset 5733e0b0e096 for causing frequent timeouts in test_XHR_timeout.html
Assignee | ||
Comment 31•7 years ago
|
||
Alright, I tried adding some debugging output on my local build to find what was causing the timeout. I never actually got the unexpected test failure on my local build, but I have some ideas of what might be causing it. When I run the test: > ./mach mochitest dom/xhr/tests/test_XHR_timeout.html on my local build, there are a lot of times when the call: > nsresult rv = internal_GetHistogramEnumId(h->histogram_name().c_str(), &id); fails (this can also be seen in the test run in comment 29). I outputted the name of each histogram (e.g., h->histogram_name()) that this failed for and attached it as a text file. There are a lot of failures (almost 4000), so I'm wondering if this could be part of the problem. The other thing I noticed is that we call internal_ShouldReflectHistogram() right before we call internal_GetHistogramEnumId(): https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/toolkit/components/telemetry/TelemetryHistogram.cpp#2277 This function also calls internal_GetHistogramEnumId() to get the id from the histogram name, which is inefficient. I don't know how long each of these calls takes, but maybe calling that twice could somehow contribute to the problem? I'm not really sure about this though.
Flags: needinfo?(olucafont6)
Comment 32•7 years ago
|
||
Good details, that helps! I think this actually uncovered a bug with this approach. We shouldn't see this many failures. So, looking through the output, there are many examples like: > sub#XMLHTTPREQUEST_ASYNC_OR_SYNC#content > CONTENT_LARGE_PAINT_PHASE_WEIGHT#dl Actually, all histogram names in that output have a "#". This is the problem: gHistogramMap is used to map the histogram names to ids. When we populate gHistogramMap in TelemetryHistogram::InitializeGlobalState(), we use the plain histogram names as they are given in Histograms.json. For each histogram in Histograms.json, we internally store multiple histograms, using different names to tell them apart. So e.g. for a keyed histogram "TEST" we can end up having these storage histograms (yes, its convoluted): TEST#someKey sub#TEST#someKey TEST#someKey#content sub#TEST#someKey#content So, we could make internal_GetHistogramMapEntry() strip the name accordingly... But actually the snapshot code is pretty old and i think we could do this much cleaner instead, using something like this instead of the current loop: > for (size_t i = 0; i < mozilla::Telemetry::HistogramCount; ++i) { > if (gHistograms[i].keyed) { > continue; > } > Histogram* h = nullptr; > internal_GetHistogramByEnumId(i, &h);
Assignee | ||
Comment 33•7 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #32) > So, we could make internal_GetHistogramMapEntry() strip the name > accordingly... > But actually the snapshot code is pretty old and i think we could do this > much cleaner instead, using something like this instead of the current loop: > > > for (size_t i = 0; i < mozilla::Telemetry::HistogramCount; ++i) { > > if (gHistograms[i].keyed) { > > continue; > > } > > Histogram* h = nullptr; > > internal_GetHistogramByEnumId(i, &h); Yeah, that does seem better. I tried this and the tests give much less errors now. We do something very similar in the previous for loop: https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/toolkit/components/telemetry/TelemetryHistogram.cpp#2239-2261 What should we be passing as the third argument (aProcessType) to GetHistogramByEnumId()? Or should we try each different value (GeckoProcessType_Default, GeckoProcessType_Content, and GeckoProcessType_GPU), perhaps in a nested for loop? Also, in the original loop we were looping over the contents of hs (a StatisticsRecorder::Histograms), but now we would just be looping over gHistograms. Just want to make sure those would be equivalent. Thanks.
Flags: needinfo?(gfritzsche)
Comment 34•7 years ago
|
||
I don't think i can help out today before i'm going into vacation. Chris, you worked on adding child process support here - could you help out?
Flags: needinfo?(gfritzsche)
Updated•7 years ago
|
Flags: needinfo?(chutten)
Comment 35•7 years ago
|
||
*lotsa reading* Looping over `gHistograms` is different since gHistograms has only one entry per histogram id. So TEST will return one Histogram*, not give you TEST#content and sub#TEST#content and so forth. "Looping" over each process type for GetHistogramByEnumId is perfectly valid (it's how that earlier for loop works, after all), so I think that it's a good thing to prototype and test. I look forward to working with you on this!
Mentor: chutten
Flags: needinfo?(chutten)
Assignee | ||
Comment 36•7 years ago
|
||
(In reply to Chris H-C :chutten from comment #35) > Looping over `gHistograms` is different since gHistograms has only one entry > per histogram id. So TEST will return one Histogram*, not give you > TEST#content and sub#TEST#content and so forth. > > "Looping" over each process type for GetHistogramByEnumId is perfectly valid > (it's how that earlier for loop works, after all), so I think that it's a > good thing to prototype and test. Okay, great, thanks. I tried this and it seems to work. I had it output the name of each histogram though, and it gave different results than the original loop (when running test_XHR_timeout.html at least). So I'm not entirely sure it's iterating over exactly the same histograms. It does seem to still pass the tests in toolkit/components/telemetry/tests/unit/ though. Thanks.
Attachment #8845187 -
Attachment is obsolete: true
Comment 37•7 years ago
|
||
Comment on attachment 8851151 [details] [diff] [review] Changed the way CreateHistogramSnapshots() iterates over histograms. Review of attachment 8851151 [details] [diff] [review]: ----------------------------------------------------------------- Did you forget to put in a r? for review? Did you want me to pass this to try? ::: toolkit/components/telemetry/TelemetryHistogram.cpp @@ +2273,5 @@ > > // OK, now we can actually reflect things. > JS::Rooted<JSObject*> hobj(cx); > + for (size_t i = 0; i < mozilla::Telemetry::HistogramCount; ++i) { > + GeckoProcessType const processTypes[] = { GeckoProcessType_Default, GeckoProcessType_Content, GeckoProcessType_GPU }; Loop invariants should be kept outside of the loop. (Compiler doesn't care, it'll do the right thing (probably), but it makes it easier to comprehend when reading (at least for me)) @@ +2274,5 @@ > // OK, now we can actually reflect things. > JS::Rooted<JSObject*> hobj(cx); > + for (size_t i = 0; i < mozilla::Telemetry::HistogramCount; ++i) { > + GeckoProcessType const processTypes[] = { GeckoProcessType_Default, GeckoProcessType_Content, GeckoProcessType_GPU }; > + size_t numTypes = (includeGPUProcess ? 3 : 2); I guess this should probably be numProcessTypes, but that's getting pedantic even for me. @@ +2276,5 @@ > + for (size_t i = 0; i < mozilla::Telemetry::HistogramCount; ++i) { > + GeckoProcessType const processTypes[] = { GeckoProcessType_Default, GeckoProcessType_Content, GeckoProcessType_GPU }; > + size_t numTypes = (includeGPUProcess ? 3 : 2); > + for (size_t type = 0; type < numTypes; ++type) { > + if (gHistograms[i].keyed) { Can move this to the outer loop, as with everything down to the internal_GetHistogramByEnumId call @@ +2292,5 @@ > + > + Histogram* original = h; > +#if !defined(MOZ_WIDGET_ANDROID) > + if (subsession) { > + h = internal_GetSubsessionHistogram(*h); Thanks to bug 1350765, you'll likely need to rebase before landing. On doing so, use the version of internal_GetSubsessionHistogram that also takes an id: it's faster.
Assignee | ||
Comment 38•7 years ago
|
||
(In reply to Chris H-C :chutten from comment #37) > Did you forget to put in a r? for review? Did you want me to pass this to > try? I actually wasn't really sure if the patch was finished yet, so wasn't sure what to ask. I was thinking about testing more to see if we were iterating over the same histograms as before. I thought I would post what I had so far though. I didn't find where StatisticsRecorder::histograms_ is populated, but I think we are probably still doing the same thing. Thanks for all of the suggestions, I updated the patch. I think we can probably push it to the try server now. > Thanks to bug 1350765, you'll likely need to rebase before landing. On doing > so, use the version of internal_GetSubsessionHistogram that also takes an > id: it's faster. Okay, sure. When the patch for that bug lands I'll update it. I wonder if adding a version of internal_ShouldReflectHistogram that also takes an ID would be a good idea, since we call it in the loop when we already have the ID. Thanks.
Attachment #8851151 -
Attachment is obsolete: true
Attachment #8851764 -
Flags: review?(chutten)
Comment 39•7 years ago
|
||
Comment on attachment 8851764 [details] [diff] [review] Implemented the changes described in comment 37. Review of attachment 8851764 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Gregory Moore [:gmoore] from comment #38) > Okay, sure. When the patch for that bug lands I'll update it. I wonder if > adding a version of internal_ShouldReflectHistogram that also takes an ID > would be a good idea, since we call it in the loop when we already have the > ID. Thanks. Sounds like an excellent plan for a follow-up bug. -- The patch looks good, I've pushed it to try over in this direction: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f2598edec1e680941eda942a1dbed15c421611f (though I accidentally pushed it atop inbound instead of central, so hopefully we won't see too much in the way of noise)
Attachment #8851764 -
Flags: review?(chutten) → review+
Comment 40•7 years ago
|
||
try is green and bug 1350765 is stalled, so you get to win the checkin race :) (sorry for the delay, was traveling for work)
Keywords: checkin-needed
Comment 41•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c6d5e6b3055d nsITelemetry.histogramSnapshots will no longer return keyed histograms. r=chutten
Keywords: checkin-needed
Comment 42•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c6d5e6b3055d
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•