Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2568303002:
Report WMPI components memory usage to UMA (Closed)
DescriptionReport WMPI components memory usage to UMA
BUG=672976
Committed: https://crrev.com/639473e49548433c259ba6639827243bcc93b3aa
Cr-Commit-Position: refs/heads/master@{#438731}
Patch Set 1 #Patch Set 2 : Call ReportMemoryUsage from WMPI::OnMemoryPressure #
Total comments: 29
Patch Set 3 : CR feedback #Patch Set 4 : Remove ReportMemoryUsageOnTimer #
Total comments: 6
Patch Set 5 : Always report memory stats to UMA + CR feedback #Patch Set 6 : buildfix #
Total comments: 2
Patch Set 7 : Drop memory pressure listener stuff for now, leave only UMA reporting #Patch Set 8 : Report only non-zero values #
Total comments: 3
Patch Set 9 : Improve UMA logging conditions #Patch Set 10 : Use histogram_suffixes in histograms.xml #
Total comments: 2
Patch Set 11 : nits #
Messages
Total messages: 73 (40 generated)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Report WMPI memory usage to UMA on memory pressure Make WMPI a memory pressure listener and report memory usage by various WMPI components when memory pressure is MODERATE or higher. BUG=672976 ========== to ========== Report WMPI memory usage to UMA on memory pressure Make WMPI a memory pressure listener and report memory usage by various WMPI components when memory pressure is MODERATE or higher. BUG=672976 ==========
servolk@chromium.org changed reviewers: + chcunningham@chromium.org, dalecurtis@chromium.org, watk@chromium.org
https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1994: if (maybe_mem_pressure_lvl && Hmm, why? Why not just always report the average memory usage? I think we should use pressure to evict, but I don't think we should be slicing the data before reporting based on it.
https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1994: if (maybe_mem_pressure_lvl && On 2016/12/12 23:49:48, DaleCurtis wrote: > Hmm, why? Why not just always report the average memory usage? I think we should > use pressure to evict, but I don't think we should be slicing the data before > reporting based on it. ReportMemoryUsage and FinishMemoryUsageReport are invoked every 2 seconds using memory_usage_reporting_timer_, not sure if we want to report to UMA that often. I'm pretty sure that we always want to report media memory usage when we are under memory pressure. So the way this CL currently implements it is we'll only report stats to UMA if this got invoked due to memory pressure being reported, and we won't report stats to UMA for regular 2-second timer updates. I can change it to always report to UMA, even for timer updates, I'm just not sure if that a good idea, WDYT?
https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1994: if (maybe_mem_pressure_lvl && On 2016/12/12 at 23:56:12, servolk wrote: > On 2016/12/12 23:49:48, DaleCurtis wrote: > > Hmm, why? Why not just always report the average memory usage? I think we should > > use pressure to evict, but I don't think we should be slicing the data before > > reporting based on it. > > ReportMemoryUsage and FinishMemoryUsageReport are invoked every 2 seconds using memory_usage_reporting_timer_, not sure if we want to report to UMA that often. I'm pretty sure that we always want to report media memory usage when we are under memory pressure. So the way this CL currently implements it is we'll only report stats to UMA if this got invoked due to memory pressure being reported, and we won't report stats to UMA for regular 2-second timer updates. I can change it to always report to UMA, even for timer updates, I'm just not sure if that a good idea, WDYT? Reporting UMA is mostly just updating a static value IIRC; certainly every 2 seconds seems fine. I don't think it actually does much on a per sample basis, though I defer to UMA folk.
I just had minor comments https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1097: // object is owned by this (WMPI::mem_pressure_listener_). "because the". Or if you want to reword it: "base::Unretained is safe because |this| outlives memory_pressure_listener_" https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1099: base::WrapUnique(new base::MemoryPressureListener(base::Bind( Prefer MakeUnique(). (Unless there's a reason you can't) https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1925: void WebMediaPlayerImpl::ReportMemoryUsageOnTimer() { Can you bind the nullopt into the callback and delete this? https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1950: const base::Optional<base::MemoryPressureListener::MemoryPressureLevel>& Use the type alias? https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1971: const int64_t data_source_mem_usage = "memory_usage" for consistency with other variables. https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1980: mem_pressure_log = " (Mem pressure: " + mem_pressure_log + ")"; Do this inside MaybeMemPressureToLogString and inline the call into the DVLOG below? https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:350: // accessed on the media thread. Also called when we get notified about memory nit: merge the "Also called when" sentence with the "Called by" above. https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:353: using MaybeMemPressureLvl = nit: It's probably more obvious if you call this OptionalMemoryPressureLevel https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:357: int64_t demuxer_memory_usage); nit: This is minor but I'd s/lvl/level and s/mem/memory everywhere. Both lvl and mem are well known enough that I think they're within the style guide recommendation for abbreviations, but they're uncommon in media code at least. (e.g., https://cs.chromium.org/search/?q=file:media/+lvl%5Cb&sq=package:chromium&typ...) Up to you though. https://codereview.chromium.org/2568303002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2568303002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25744: +<histogram name="Media.WebMediaPlayerImpl.Memory.AudioKb" units="KB"> I don't think putting Kb in the name is conventional because you have the units as well. https://codereview.chromium.org/2568303002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25747: + Amound of memory used by WebMediaPlayerImpl audio renderer when memory Amount. "used by the"
On 2016/12/13 00:04:07, DaleCurtis wrote: > https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... > media/blink/webmediaplayer_impl.cc:1994: if (maybe_mem_pressure_lvl && > On 2016/12/12 at 23:56:12, servolk wrote: > > On 2016/12/12 23:49:48, DaleCurtis wrote: > > > Hmm, why? Why not just always report the average memory usage? I think we > should > > > use pressure to evict, but I don't think we should be slicing the data > before > > > reporting based on it. > > > > ReportMemoryUsage and FinishMemoryUsageReport are invoked every 2 seconds > using memory_usage_reporting_timer_, not sure if we want to report to UMA that > often. I'm pretty sure that we always want to report media memory usage when we > are under memory pressure. So the way this CL currently implements it is we'll > only report stats to UMA if this got invoked due to memory pressure being > reported, and we won't report stats to UMA for regular 2-second timer updates. I > can change it to always report to UMA, even for timer updates, I'm just not sure > if that a good idea, WDYT? > > Reporting UMA is mostly just updating a static value IIRC; certainly every 2 > seconds seems fine. I don't think it actually does much on a per sample basis, > though I defer to UMA folk. Ok, but even performance concerns aside for a moment. How useful would it be for us to have the average memory usage by WMPI under non-memory-pressure conditions? Do we expect to learn anything useful from such an UMA stat? I understand how memory usage under memory pressure could be interesting. We could log average memory usage when the memory pressure was first reported and then log average memory usage again after doing more aggressive MSE GC or releasing memory in some other way. Then by comparing those two metrics we would be able to see how much of an improvement in memory usage we got. But what are we going to compase the average memory usage under normal conditions to? Btw, I was thinking that perhaps in addition to logging raw memory usage numbers it might be interesting to also log what proportion of memory is taken by WMPI in proportion to all available memory. That could allow us to highlight memory savings even better on low-memory platforms. WDYT? Ideally we would probably also want to see what proportion of available memory is being used by ALL WMPI instances, and not by just one WMPI instance. But I'm not sure how to do that yet. Is there a way we could somehow iterate over all WMPI instances from the browser process? If we could do that, we could probably add a media entry in MetricsMemoryDetails::UpdateHistograms in chrome/browser/metrics/metrics_memory_details.cc.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1097: // object is owned by this (WMPI::mem_pressure_listener_). On 2016/12/13 23:34:31, watk wrote: > "because the". > > Or if you want to reword it: "base::Unretained is safe because |this| outlives > memory_pressure_listener_" Done. https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1099: base::WrapUnique(new base::MemoryPressureListener(base::Bind( On 2016/12/13 23:34:31, watk wrote: > Prefer MakeUnique(). (Unless there's a reason you can't) Done. https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1925: void WebMediaPlayerImpl::ReportMemoryUsageOnTimer() { On 2016/12/13 23:34:31, watk wrote: > Can you bind the nullopt into the callback and delete this? This is passed into timer.Start (see https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?rcl=0...). For some reason the timer start API is using a function pointer and an object pointer instead of something like base::Closure (see https://cs.chromium.org/chromium/src/base/timer/timer.h?rcl=1481645941&l=238). I'm not sure why that is, but I'd prefer not to change base::Timer API in this CL, so leaving it as it is for now. https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1950: const base::Optional<base::MemoryPressureListener::MemoryPressureLevel>& On 2016/12/13 23:34:31, watk wrote: > Use the type alias? I could, but the type alias is currently private in WMPI. Do you think it's worth making the type alias public just to use it here? https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1971: const int64_t data_source_mem_usage = On 2016/12/13 23:34:31, watk wrote: > "memory_usage" for consistency with other variables. Done. https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1980: mem_pressure_log = " (Mem pressure: " + mem_pressure_log + ")"; On 2016/12/13 23:34:31, watk wrote: > Do this inside MaybeMemPressureToLogString and inline the call into the DVLOG > below? This would make MaybeMemPressureToLogString less re-usable (usable only in this context). Perhaps we'll want to convert MemoryPressure enum into string in somewhere else, where we don't want to add this log comment to it. https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:350: // accessed on the media thread. Also called when we get notified about memory On 2016/12/13 23:34:31, watk wrote: > nit: merge the "Also called when" sentence with the "Called by" above. Done. https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:353: using MaybeMemPressureLvl = On 2016/12/13 23:34:31, watk wrote: > nit: It's probably more obvious if you call this OptionalMemoryPressureLevel Done. https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:357: int64_t demuxer_memory_usage); On 2016/12/13 23:34:31, watk wrote: > nit: This is minor but I'd s/lvl/level and s/mem/memory everywhere. Both lvl and > mem are well known enough that I think they're within the style guide > recommendation for abbreviations, but they're uncommon in media code at least. > (e.g., > https://cs.chromium.org/search/?q=file:media/+lvl%5Cb&sq=package:chromium&typ...) > > Up to you though. Done. https://codereview.chromium.org/2568303002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2568303002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25744: +<histogram name="Media.WebMediaPlayerImpl.Memory.AudioKb" units="KB"> On 2016/12/13 23:34:31, watk wrote: > I don't think putting Kb in the name is conventional because you have the units > as well. I've noticed that some metrics in this file use Kb suffix even despite having units=KB, e.g. Compositing.Renderer.PictureMemoryUsageKb or PurgeAndSuspend.Memory.PartitionAllocKB or Sqlite.SizeKB, but you are right, most of them don't use the Kb suffix, so dropped it. https://codereview.chromium.org/2568303002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25747: + Amound of memory used by WebMediaPlayerImpl audio renderer when memory On 2016/12/13 23:34:31, watk wrote: > Amount. > > "used by the" Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1925: void WebMediaPlayerImpl::ReportMemoryUsageOnTimer() { On 2016/12/14 00:07:23, servolk wrote: > On 2016/12/13 23:34:31, watk wrote: > > Can you bind the nullopt into the callback and delete this? > > This is passed into timer.Start (see > https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?rcl=0...). > For some reason the timer start API is using a function pointer and an object > pointer instead of something like base::Closure (see > https://cs.chromium.org/chromium/src/base/timer/timer.h?rcl=1481645941&l=238). > I'm not sure why that is, but I'd prefer not to change base::Timer API in this > CL, so leaving it as it is for now. Ah, wait, despite the weird Start method at https://cs.chromium.org/chromium/src/base/timer/timer.h?rcl=1481645941&l=238 there is also a friendlier (taking base::Closure) Timer::Start in the base class (https://cs.chromium.org/chromium/src/base/timer/timer.h?rcl=1481645941&l=110). So maybe I will be able to get rid of this. I'll give it a try in the next patchset.
https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1925: void WebMediaPlayerImpl::ReportMemoryUsageOnTimer() { On 2016/12/14 00:11:23, servolk wrote: > On 2016/12/14 00:07:23, servolk wrote: > > On 2016/12/13 23:34:31, watk wrote: > > > Can you bind the nullopt into the callback and delete this? > > > > This is passed into timer.Start (see > > > https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?rcl=0...). > > For some reason the timer start API is using a function pointer and an object > > pointer instead of something like base::Closure (see > > https://cs.chromium.org/chromium/src/base/timer/timer.h?rcl=1481645941&l=238). > > I'm not sure why that is, but I'd prefer not to change base::Timer API in this > > CL, so leaving it as it is for now. > > Ah, wait, despite the weird Start method at > https://cs.chromium.org/chromium/src/base/timer/timer.h?rcl=1481645941&l=238 > there is also a friendlier (taking base::Closure) Timer::Start in the base class > (https://cs.chromium.org/chromium/src/base/timer/timer.h?rcl=1481645941&l=110). > So maybe I will be able to get rid of this. I'll give it a try in the next > patchset. Done.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Implementation lgtm. I'll leave you and Dale to discuss when we want to record the UMAs. It seems to me that every 2 seconds would not be a performance concern at all. But I'm not sure what kinds of questions we can answer with one set of data vs the other. https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1950: const base::Optional<base::MemoryPressureListener::MemoryPressureLevel>& On 2016/12/14 00:07:23, servolk wrote: > On 2016/12/13 23:34:31, watk wrote: > > Use the type alias? > > I could, but the type alias is currently private in WMPI. Do you think it's > worth making the type alias public just to use it here? Ah, didn't notice this was a non-member function. Leaving it sgtm https://codereview.chromium.org/2568303002/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2568303002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1947: std::string MaybeMemPressureToLogString( If you want this to be reusable (which I personally wouldn't worry about at the moment), you'd be better off taking a MemoryPressureLevel, not an optional. MemoryPressureLevelToString()? "Maybe" is out of date. "Log" feels redundant. https://codereview.chromium.org/2568303002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1993: *memory_pressure_level >= Doesn't really matter, but you don't need the deref (https://cs.chromium.org/chromium/src/base/optional.h?q=base::optional&sq=pack...) https://codereview.chromium.org/2568303002/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2568303002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:368: void OnMemoryPressure(base::MemoryPressureListener::MemoryPressureLevel lvl); name doesn't match cc.
dalecurtis@chromium.org changed required reviewers: + dalecurtis@chromium.org
You should never pre-slice your data like this ahead of recording; i.e. what's memory pressure on one device vs another will be different and definitely not what we expect as a reasonable average across all devices. What we want to do is always record the average and then launch a finch trial which opts-in some percentage of the population into our memory pressure experiment. We'll notice changes in the average if it's working properly. Alternatively if we always think this is going to yield a better experience, just launching this ahead of the feature and watching the UMA average drop is also good; though not as rigorous. marked myself as a required reviewer so this doesn't land until that's resolved.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/14 00:44:37, DaleCurtis_OOO_Dec14_Jan6 wrote: > You should never pre-slice your data like this ahead of recording; i.e. what's > memory pressure on one device vs another will be different and definitely not > what we expect as a reasonable average across all devices. > > What we want to do is always record the average and then launch a finch trial > which opts-in some percentage of the population into our memory pressure > experiment. We'll notice changes in the average if it's working properly. > > Alternatively if we always think this is going to yield a better experience, > just launching this ahead of the feature and watching the UMA average drop is > also good; though not as rigorous. > > marked myself as a required reviewer so this doesn't land until that's resolved. I wouldn't call it pre-slicing the data. It's just that we are not going to do anything if there's no memory pressure, so logging this data when there's no memory pressure seemed redundant to me. But sure, if you guys think it's useful to always log this, we can do this (updated in the latest patchset). I agree that we should still see improvement on average, it's just going to be on a smaller scale, since there's not going to be any improvement on systems where we didn't experience the memory pressure. Like I've mentioned in the previous message perhaps a better metric that would allow us to better see gains from this is the proportion of WMPI-allocated memory to all available memory.
https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2568303002/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1950: const base::Optional<base::MemoryPressureListener::MemoryPressureLevel>& On 2016/12/14 00:34:05, watk wrote: > On 2016/12/14 00:07:23, servolk wrote: > > On 2016/12/13 23:34:31, watk wrote: > > > Use the type alias? > > > > I could, but the type alias is currently private in WMPI. Do you think it's > > worth making the type alias public just to use it here? > > Ah, didn't notice this was a non-member function. Leaving it sgtm Acknowledged. https://codereview.chromium.org/2568303002/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2568303002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1947: std::string MaybeMemPressureToLogString( On 2016/12/14 00:34:05, watk wrote: > If you want this to be reusable (which I personally wouldn't worry about at the > moment), you'd be better off taking a MemoryPressureLevel, not an optional. > MemoryPressureLevelToString()? "Maybe" is out of date. "Log" feels redundant. Done. https://codereview.chromium.org/2568303002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1993: *memory_pressure_level >= On 2016/12/14 00:34:05, watk wrote: > Doesn't really matter, but you don't need the deref > (https://cs.chromium.org/chromium/src/base/optional.h?q=base::optional&sq=pack...) Done. https://codereview.chromium.org/2568303002/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.h (right): https://codereview.chromium.org/2568303002/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.h:368: void OnMemoryPressure(base::MemoryPressureListener::MemoryPressureLevel lvl); On 2016/12/14 00:34:05, watk wrote: > name doesn't match cc. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Drop all the memory pressure stuff from this CL since we're always recording it. Note: You can slice based on RAM, low-end devices, etc in UMA. https://codereview.chromium.org/2568303002/diff/100001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2568303002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1997: stats.video_memory_usage / 1024); Typo?
servolk@chromium.org changed reviewers: + isherman@chromium.org
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2568303002/diff/100001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2568303002/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1997: stats.video_memory_usage / 1024); On 2016/12/14 19:33:23, DaleCurtis_OOO_Dec14_Jan6 wrote: > Typo? Oops, yes, fixed.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/14 19:33:23, DaleCurtis_OOO_Dec14_Jan6 wrote: > Drop all the memory pressure stuff from this CL since we're always recording it. > Note: You can slice based on RAM, low-end devices, etc in UMA. > > https://codereview.chromium.org/2568303002/diff/100001/media/blink/webmediapl... > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/2568303002/diff/100001/media/blink/webmediapl... > media/blink/webmediaplayer_impl.cc:1997: stats.video_memory_usage / 1024); > Typo? Done, removed memory pressure stuff for now. +isherman@ for tools/metrics/histograms/histograms.xml
Since we may not have audio or video tracks you may want to only record those values if such a track exists, otherwise your average will be weighted towards zero.
On 2016/12/14 at 19:54:27, DaleCurtis_OOO_Dec14_Jan6 wrote: > Since we may not have audio or video tracks you may want to only record those values if such a track exists, otherwise your average will be weighted towards zero. Ditto for DataSource.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
On 2016/12/14 19:54:41, DaleCurtis_OOO_Dec14_Jan6 wrote: > On 2016/12/14 at 19:54:27, DaleCurtis_OOO_Dec14_Jan6 wrote: > > Since we may not have audio or video tracks you may want to only record those > values if such a track exists, otherwise your average will be weighted towards > zero. > > Ditto for DataSource. Done
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Metrics LGTM https://codereview.chromium.org/2568303002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2568303002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:25887: +</histogram> Optional: You might want to use a histogram_suffixes element to reduce the amount of repetition in this file.
https://codereview.chromium.org/2568303002/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2568303002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1953: if (stats.audio_memory_usage > 0) { I think you want hasAudio(), hasVideo(), hasDataSource() equivalents, not the measure of zero, since it's possible at some points to have a valid 0 right?
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2568303002/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2568303002/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1953: if (stats.audio_memory_usage > 0) { On 2016/12/14 20:38:55, DaleCurtis_OOO_Dec14_Jan6 wrote: > I think you want hasAudio(), hasVideo(), hasDataSource() equivalents, not the > measure of zero, since it's possible at some points to have a valid 0 right? Done.
Description was changed from ========== Report WMPI memory usage to UMA on memory pressure Make WMPI a memory pressure listener and report memory usage by various WMPI components when memory pressure is MODERATE or higher. BUG=672976 ========== to ========== Report WMPI components memory usage to UMA BUG=672976 ==========
lgtm, I recommend the histogram suffixes change that isherman@ suggested though.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/14 20:09:48, Ilya Sherman wrote: > Metrics LGTM > > https://codereview.chromium.org/2568303002/diff/140001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2568303002/diff/140001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:25887: +</histogram> > Optional: You might want to use a histogram_suffixes element to reduce the > amount of repetition in this file. Done. Can you please take one more quick look at histograms.xml to ensure I'm using the histogram_suffixes correctly?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Metrics still lgtm https://codereview.chromium.org/2568303002/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2568303002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:25858: +<!-- Name completed by histogram_suffixes name="MediaWMPIMemoryUsage" --> nit: Please omit this comment, and instead add base="true" on the line above.
https://codereview.chromium.org/2568303002/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2568303002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:25858: +<!-- Name completed by histogram_suffixes name="MediaWMPIMemoryUsage" --> On 2016/12/15 01:30:54, Ilya Sherman wrote: > nit: Please omit this comment, and instead add base="true" on the line above. Done. Thanks!
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org, dalecurtis@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2568303002/#ps200001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by servolk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by servolk@chromium.org
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": 200001, "attempt_start_ts": 1481770081580470,
"parent_rev": "9e6d57ab70faef0ea807fc53871b4c6bd38750df", "commit_rev":
"c5ae58c499e3f53137ba76a18e0b4d0c8f0d356d"}
Message was sent while issue was closed.
Description was changed from ========== Report WMPI components memory usage to UMA BUG=672976 ========== to ========== Report WMPI components memory usage to UMA BUG=672976 Review-Url: https://codereview.chromium.org/2568303002 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Report WMPI components memory usage to UMA BUG=672976 Review-Url: https://codereview.chromium.org/2568303002 ========== to ========== Report WMPI components memory usage to UMA BUG=672976 Committed: https://crrev.com/639473e49548433c259ba6639827243bcc93b3aa Cr-Commit-Position: refs/heads/master@{#438731} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/639473e49548433c259ba6639827243bcc93b3aa Cr-Commit-Position: refs/heads/master@{#438731} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
