Closed
Bug 1381470
Opened 7 years ago
Closed 7 years ago
Reduce wastefully allocations for internal_IsEmpty() in TelemetryHistogram.cpp
Categories
(Toolkit :: Telemetry, enhancement, P4)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: gfritzsche, Assigned: deanzhu1, Mentored)
References
Details
(Whiteboard: [lang=c++])
Attachments
(1 file, 2 obsolete files)
1.66 KB,
patch
|
Dexter
:
review+
|
Details | Diff | Splinter Review |
malloc analysis flagged internal_IsEmpty() for large stack allocations with a lifetime of few instructions: https://dxr.mozilla.org/mozilla-central/rev/5f44d10bacca2d693413b529e0caadc73e634e1e/toolkit/components/telemetry/TelemetryHistogram.cpp#231 This is due to mainly unused sampleset allocation. We should simply add a "is empty" method to base::Histogram instead.
Comment 1•7 years ago
|
||
The blocks die very young (169 instructions) and also they are almost entirely un-read (0.01 read ratio, meaning on average only 1% of them is ever read.) -------------------- 66 of 1000 -------------------- max-live: 40,000 in 1 blocks tot-alloc: 5,178,672 in 18,976 blocks (avg size 272.90) deaths: 18,976, at avg age 169 (0.00% of prog lifetime) acc-ratios: 0.01 rd, 1.11 wr (75,904 b-read, 5,768,384 b-written) at 0x4C2BF9D: malloc (vg_replace_malloc.c:299) by 0x4058D3: moz_xmalloc (mozalloc.cpp:83) by 0x10BC1A0B: operator new (mozalloc.h:194) by 0x10BC1A0B: allocate (new_allocator.h:104) by 0x10BC1A0B: allocate (alloc_traits.h:416) by 0x10BC1A0B: _M_allocate (stl_vector.h:170) by 0x10BC1A0B: _M_allocate_and_copy<__gnu_cxx::__normal_iterator<int const*, std::vector<int> > > (stl_vector.h:1222) by 0x10BC1A0B: std::vector<int, std::allocator<int> >::operator=(std::vector<int, std::allocator<int> > const&) (vector.tcc:195) by 0x10BB91D2: operator= (histogram.h:321) by 0x10BB91D2: base::Histogram::SnapshotSample(base::Histogram::SampleSet*) const (histogram.cc:299) by 0x13A3DA23: internal_IsEmpty (TelemetryHistogram.cpp:232) by 0x13A3DA23: TelemetryHistogram::CreateHistogramSnapshots(JSContext*, JS::MutableHandle<JS::Value>, bool, bool) (TelemetryHistogram.cpp\ :2258) by 0x13A2BBD2: (anonymous namespace)::TelemetryImpl::SnapshotSubsessionHistograms(bool, JSContext*, JS::MutableHandle<JS::Value>) (Teleme\ try.cpp:852) by 0x1078F1DD: ??? (xptcinvoke_asm_x86_64_unix.S:106) by 0x11174AB9: Invoke (XPCWrappedNative.cpp:1996) by 0x11174AB9: Call (XPCWrappedNative.cpp:1315) by 0x11174AB9: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:1282) by 0x11179AEE: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:967) by 0x13C4DD04: CallJSNative (jscntxtinlines.h:293) by 0x13C4DD04: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:470)
Reporter | ||
Updated•7 years ago
|
Hello, I'd like to help with this bug. I fixed a few bugs with the tag [good first bugs] (1352564 and 1347157 for instance) and I wondered if I'm fit to fix this one. If so, could you please give me a little overview on this bug? Thank you!
Comment 3•7 years ago
|
||
(In reply to Dean Zhu from comment #2) > Hello, I'd like to help with this bug. > I fixed a few bugs with the tag [good first bugs] (1352564 and 1347157 for > instance) and I wondered if I'm fit to fix this one. > If so, could you please give me a little overview on this bug? > Thank you! Chris, would you kindly provide more details about this to get Dean started?
Flags: needinfo?(chutten)
Comment 4•7 years ago
|
||
The main thrust of this bug is that, in certain circumstances, we call internal_IsEmpty() in TelemetryHistogram.cpp. When we do, we allocate a full Histogram::SampleSet object just to copy the Histogram's information into it and see if it is empty. This bug is about adding functionality to base::Histogram so that we don't have to go through all this effort just to see if someone's added information to the Histogram. So, crack open histogram.h [1] and .cc [2] and implement Histogram::is_empty() (using Chromium style because this is code adapted from the Chromium project) which returns true if and only if there is data being stored in the Histogram. If you think this is something you'd like to try out, please let me know and I'll assign it to you. Also: please let me know via the needinfo? mechanism of any questions you might have. I'd be happy to answer them. [1]: https://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/histogram.h [2]: https://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/histogram.cc
Flags: needinfo?(chutten)
Thank you, I definitely would like to help fixing this bug. Let me know if I got this correct: Currently we use a method called internal_IsEmpty() in telemetryhistogram.cpp, which allocates too much memory. Now we want a new method called is_empty() in the base::Histogram class to fix this. And for example we should change [1] to not call internal_IsEmpty() and instead call h.is_Empty()? Thank you! [1] : https://dxr.mozilla.org/mozilla-central/rev/5f44d10bacca2d693413b529e0caadc73e634e1e/toolkit/components/telemetry/TelemetryHistogram.cpp#2263
Flags: needinfo?(chutten)
Comment 6•7 years ago
|
||
Or have internal_IsEmpty() call h.is_empty() for us, exactly correct.
Assignee: nobody → deanzhu1
Flags: needinfo?(chutten)
I think this should fix the bugs mentioned above. Tell me if there is anything I should change.
Attachment #8891100 -
Flags: review?(chutten)
Comment 8•7 years ago
|
||
Comment on attachment 8891100 [details] [diff] [review] Bug 1381470 Review of attachment 8891100 [details] [diff] [review]: ----------------------------------------------------------------- This is exactly what's needed, thank you! There's just one small problem (we sometimes call them 'nits') and then this will be good to go. ::: ipc/chromium/src/base/histogram.h @@ +114,5 @@ > }; > > size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf); > > + bool is_Empty() const { chromium style is snake_case for member functions, so this will need a lowercase 'e'.
Attachment #8891100 -
Flags: review?(chutten) → review-
Fixed snakecase
Attachment #8891632 -
Flags: review?(chutten)
Updated•7 years ago
|
Attachment #8891100 -
Attachment is obsolete: true
Comment 10•7 years ago
|
||
Comment on attachment 8891632 [details] [diff] [review] Bug1381470.diff Review of attachment 8891632 [details] [diff] [review]: ----------------------------------------------------------------- Hi Dean! I'm taking over the review of this as Chris is not available this week. Good job with fixing the nit he pointed to! I have a couple of observations: - Could you please remove the trailing whitespaces mentioned below? - Could you please change the commit message to "Bug 1381470 - Added a less memory consuming method to tell if a histogram is empty. r?chutten". The format of the message can be found at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Commit_message_restrictions Please feel free to flag me for review during this week instead of Chris :) Let me know if you have any question! ::: ipc/chromium/src/base/histogram.h @@ +117,5 @@ > > + bool is_empty() const { > + return this->sample_.counts(0) == 0 && this->sample_.sum() == 0; > + } > + nit: can you remove the trailing whitespaces on this line?
Attachment #8891632 -
Flags: review?(chutten) → feedback+
Assignee | ||
Comment 11•7 years ago
|
||
The trailing spaces should be fixed now.
Attachment #8891632 -
Attachment is obsolete: true
Attachment #8892147 -
Flags: review?(alessio.placitelli)
Comment 12•7 years ago
|
||
Comment on attachment 8892147 [details] [diff] [review] Bug1381470.diff Review of attachment 8892147 [details] [diff] [review]: ----------------------------------------------------------------- This looks good now, thanks!
Attachment #8892147 -
Flags: review?(alessio.placitelli) → review+
Comment 13•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9741ce8bb6b3fc56868a5b245d21e31630937e59
Updated•7 years ago
|
Keywords: checkin-needed
Comment 14•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/91c93c5ea5b2 Added a less memory consuming method to tell if a histogram is empty. r=chutten
Keywords: checkin-needed
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/91c93c5ea5b2
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•