Pending pings can take up memory

RESOLVED WORKSFORME

Status

()

defect
RESOLVED WORKSFORME
5 years ago
4 years ago

People

(Reporter: jchen, Unassigned)

Tracking

(Blocks 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec+)

Details

Attachments

(1 attachment)

We load pending telemetry pings on startup and keep them in memory until they are sent during the next daily idle notification. However, if there are a lot of pending pings, they can take up quite a bit of memory without being used for anything. We should load pending pings only when we are about to send them.

My Fennec profile had 17 pending pings and about:memory said they were taking up almost 16MB of memory. Looking at telemetry.mozilla.org, this may be an extreme case because most users seem to have less number of pending pings, but I think it's still a good way to save some memory.
tracking-fennec: --- → ?
Richard - Can you investigate this
Assignee: nobody → rnewman
tracking-fennec: ? → +
Status: NEW → ASSIGNED
TelemetryPing.jsm asserts that:

        yield TelemetryFile.loadSavedPings();
        // If we have any TelemetryPings lying around, we'll be aggressive
        // and try to send them all off ASAP.
        if (TelemetryFile.pingsOverdue > 0) {

pingsOverdue is defined as older than one week, so we should have no more than one week's worth of data waiting to be uploaded -- anything older will be uploaded on startup.

Anything older than two weeks will be deleted.

That implies that we're producing a lot of telemetry in a week, no? More than two slugs per day, 1MB per slug.


Regardless, I think that this desktop assumption is flawed: we shouldn't load every file from disk just to figure out if any are stale and wait to upload them.
My proposal:

* Don't call loadSavedPings on startup. Instead, approximate whether there are any overdue slugs -- either by walking the filesystem as we already do, or by tracking more metadata -- and if there are, loading *only the overdue ones* for immediate upload.

* Either:
** Make popPayloads loading pending pings on demand, keeping TelemetryFile mostly untouched.
** Make popPendingPings invoke loadHistograms, rather than doing it in loadSavedPings. lSP would only load metadata.

In either case, TelemetryFile would no longer be an in-memory clone of on-disk ping files; it'd be an index.


rvitillo and mconley have done work on this particular functionality. What do you folks think?

Feel free to steal this from me if you'd rather me not monkey around!
Flags: needinfo?(rvitillo)
Flags: needinfo?(mconley)
Depends on: 863872
(In reply to Richard Newman [:rnewman] from comment #3)
> My proposal:
> 
> * Don't call loadSavedPings on startup. Instead, approximate whether there
> are any overdue slugs -- either by walking the filesystem as we already do,
> or by tracking more metadata -- and if there are, loading *only the overdue
> ones* for immediate upload.
> 
> * Either:
> ** Make popPayloads loading pending pings on demand, keeping TelemetryFile
> mostly untouched.
> ** Make popPendingPings invoke loadHistograms, rather than doing it in
> loadSavedPings. lSP would only load metadata.
> 
> In either case, TelemetryFile would no longer be an in-memory clone of
> on-disk ping files; it'd be an index.
> 
> 
> rvitillo and mconley have done work on this particular functionality. What
> do you folks think?

The second approach looks cleaner to me. But keep in mind that recently a patch landed that limits the maximum number of saved pings to 17 and that the average size of a ping is 60KB on Nightly. That being said, if you feel that this issue is still cause for concern, feel free to go ahead and implement the changes :)
Flags: needinfo?(rvitillo)
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #4)

Thanks for the quick response!


> The second approach looks cleaner to me. But keep in mind that recently a
> patch landed that limits the maximum number of saved pings to 17 and that
> the average size of a ping is 60KB on Nightly.

That helps, for sure, but that still devotes up to 1MB (or more if you're jchen, apparently) of our incredibly precious phone memory to telemetry pings -- we'd like that number to be zero most of the time, and peak at 60KB during the actual one-by-one loading of slugs.

On some devices we're one big favicon away from an OutOfMemoryException, so we have to be very mindful of non-critical allocations.

> feel free to go ahead and
> implement the changes :)

Thanks!
I think an index based on write date makes sense. As you say, the actual contents of the files is uninteresting until we need to actually send them, so holding onto that stuff in memory if we don't need to send them is wasteful.

So, thumbs up on the idea from me. :)
Flags: needinfo?(mconley)
And 60KB is the file size, right? IME it takes up more than that once it's parsed into memory.
I think this is a dupe of bug 833545. I was planning on working on this myself, but I can also just help with reviews.

Jim, can you attach or send me an example of your oversized pings?
Flags: needinfo?(nchen)
Ah looks like a dupe. I see 18MB under TelemetryFile.jsm (in about:memory) when I put these in my saved-telemetry-pings directory, and nothing once I delete them.
Flags: needinfo?(nchen)
Duplicate of this bug: 833545
Vladan, if you'll have time to work on this in the next cycle or two, I'd be happy to hand this over. Steal away!
filter on [mass-p5]
Priority: -- → P5
Assignee: rnewman → nobody
Priority: P5 → --
Status: ASSIGNED → NEW
Depends on: 1126986
Duplicate of this bug: 1126986
vladan, you were working on a patch for bug 1126986, which was marked as a dupe of this.
Should this be assigned to you?
Flags: needinfo?(vdjeric)
Vladan was working on a patch in bug 1126986.
Assignee: nobody → vdjeric
Vladan has this WIP.
Flags: needinfo?(vdjeric)
Bug 1156355 tracks consolidating ping file load logic etc. in TelemetryStorage, which might have some overlap with this bug.
Assignee: vdjeric → nobody
Bug 1156359 refactors the ping sending logic and fixes keeping the pings in memory.
If we don't succeed in sending pings off, we save them to disk and load them from there for the next send attempt.

Vladan, was there still something about excessive memory usage per ping that you were looking into?
Flags: needinfo?(vdjeric)
(In reply to Georg Fritzsche [:gfritzsche] from comment #19)
> Vladan, was there still something about excessive memory usage per ping that
> you were looking into?

Per bug 1126986, comment 3, this is fine.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(vdjeric)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.