Closed Bug 836186 Opened 7 years ago Closed 7 years ago

Investigate memory overhead for providers and measurements

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 21

People

(Reporter: gps, Assigned: gps)

References

Details

(Whiteboard: [MemShrink])

Attachments

(1 file)

I temporarily disabled all FHR providers (current count is 6) in a local build and observed their shared JS compartment decrease in size by ~133kb. This surprised me as ~22kb per compartment seemed a bit steep to me. I wonder how much of this was per-object overhead vs overhead for compiled code, etc.

I think we should investigate whether just-in-time creating providers (at least the ones that provide constant data) nets a significant memory win. If so, this should be a relatively low hanging fruit.

If per JS object overhead is high, we should see about reducing it, as the number of providers will only grow over time.
This saves ~53k by my measurements.

While I was measuring the effect, I obtained the following numbers:

464 as currently implemented
384 don't create provider instances
389 create and delete instances
419 constructor and populate prefs and other small properties
411 this patch
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #708202 - Flags: review?(rnewman)
Whiteboard: [MemShrink]
Comment on attachment 708202 [details] [diff] [review]
Only load constant providers at collection time, v1

Review of attachment 708202 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: services/healthreport/healthreporter.jsm
@@ +591,5 @@
> +        try {
> +          let provider = this.initProviderFromType(providerType);
> +          yield this.registerProvider(provider);
> +        } catch (ex) {
> +          this._log.warn("Error registering constant only provider: " +

Hyphenate constant-only for clarity.

@@ +598,5 @@
> +      }
> +
> +      let result;
> +      try {
> +        result = yield this._collector.collectConstantData();

Something to think about: can we get to the point of doing this only once, so that we can collect this once, and afterwards discard _constantOnlyProviders and avoid re-building each time?

(And is it worth doing so?)

@@ +605,5 @@
> +          if (!provider.constantOnly) {
> +            continue;
> +          }
> +
> +          this._log.info("Shutting down constant only provider: " +

Hyphenate constant-only for clarity.
Attachment #708202 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/services/services-central/rev/3446cda2c8b8
Whiteboard: [MemShrink] → [MemShrink][fixed in services]
(In reply to Richard Newman [:rnewman] from comment #2)
> @@ +598,5 @@
> > +      }
> > +
> > +      let result;
> > +      try {
> > +        result = yield this._collector.collectConstantData();
> 
> Something to think about: can we get to the point of doing this only once,
> so that we can collect this once, and afterwards discard
> _constantOnlyProviders and avoid re-building each time?
> 
> (And is it worth doing so?)

Yes. However, at current, collectConstantData really means "collect data at upload time" and this is being used as "collect data every ~24h."

We need to introduce some new collect* APIs.
https://hg.mozilla.org/mozilla-central/rev/3446cda2c8b8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink][fixed in services] → [MemShrink]
Target Milestone: --- → mozilla21
Blocks: 838827
Component: Metrics and Firefox Health Report → Client: Desktop
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla21 → Firefox 21
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.