Closed Bug 1381470 Opened 2 years ago Closed 2 years ago

Reduce wastefully allocations for internal_IsEmpty() in TelemetryHistogram.cpp

Categories

(Toolkit :: Telemetry, enhancement, P4)

enhancement

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)

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.
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)
Depends on: 1366294
No longer depends on: 1362953
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!
(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)
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)
Or have internal_IsEmpty() call h.is_empty() for us, exactly correct.
Assignee: nobody → deanzhu1
Flags: needinfo?(chutten)
Attached patch Bug 1381470 (obsolete) — Splinter Review
I think this should fix the bugs mentioned above. Tell me if there is anything I should change.
Attachment #8891100 - Flags: review?(chutten)
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-
Attached patch Bug1381470.diff (obsolete) — Splinter Review
Fixed snakecase
Attachment #8891632 - Flags: review?(chutten)
Attachment #8891100 - Attachment is obsolete: true
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+
Attached patch Bug1381470.diffSplinter Review
The trailing spaces should be fixed now.
Attachment #8891632 - Attachment is obsolete: true
Attachment #8892147 - Flags: review?(alessio.placitelli)
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+
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
https://hg.mozilla.org/mozilla-central/rev/91c93c5ea5b2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.