Closed Bug 1287833 Opened 3 years ago Closed 3 years ago

TelemetryStorage.jsm _archivedPings map could be more memory efficient

Categories

(Toolkit :: Telemetry, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: bkelly, Assigned: bkelly, Mentored)

Details

(Whiteboard: [measurement:client] [lang=js] [MemShrink])

Attachments

(1 file, 1 obsolete file)

Currently TelemetryStorage.jsm keeps an in-memory map of all telemetry pings it knows about.  The map looks like:

  // Tracks the pending pings in a Map of (id -> {timestampCreated, type}).
  // We use this to cache info on pending pings to avoid scanning the disk more than once.
  _pendingPings: new Map(),

This doesn't grow too large, but we do get some pathological behavior with the type strings.  From my about:memory:

  0.33 MB (00.14%) ── string(length=4, copies=14361, "sync")/gc-heap/latin1

We could improve this by de-duplicating the type string stored in the map.  I believe this can be done with this string intern'ing function:

  (str) => Symbol.keyFor(Symbol.for(str))
Mentor: gfritzsche
OS: Unspecified → All
Priority: -- → P3
Hardware: Unspecified → All
Whiteboard: [MemShrink] → [measurement:client] [lang=js] [MemShrink]
Version: unspecified → Trunk
Note that the specific case of the pathological sync ping submission is being addressed in bug 1287473 et al.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Attachment #8772484 - Flags: review?(gfritzsche)
I verified that multiple "sync" strings are not created by inspecting the gc/cc logs locally.
Comment on attachment 8772484 [details] [diff] [review]
Make TelemetryStorage intern the type string when building _archivedPings m ap. r=gfritzsche

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

Thanks Ben!

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +120,5 @@
>    let promises = Array.from(it, p => p.catch(dummy));
>    return Promise.all(promises);
>  }
>  
> +function internString(str) {

Please put a comment here on why we do this.
Attachment #8772484 - Flags: review?(gfritzsche) → feedback+
Added a comment as requested.
Attachment #8772484 - Attachment is obsolete: true
Attachment #8773007 - Flags: review?(gfritzsche)
Comment on attachment 8773007 [details] [diff] [review]
Make TelemetryStorage intern the type string when building _archivedPings map. r=gfritzsche

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

Thanks!

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +122,5 @@
>  }
>  
> +/**
> + * Permanently intern the given string.  This is mainly used for the ping.type
> + * strings we can be excessively duplicated in the _archivedPings map.  Do not

Nits:
* "we can" -> "that can"
* double space before "Do not"
Attachment #8773007 - Flags: review?(gfritzsche) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9206985d615d
Make TelemetryStorage intern the type string when building _archivedPings map. r=gfritzsche
https://hg.mozilla.org/mozilla-central/rev/9206985d615d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.