1041616 - Pending pings can take up memory

Status: RESOLVED WORKSFORME

(Toolkit :: Telemetry, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Richard - Can you investigate this

Comment 2

[Richard Newman [:rnewman]]

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.

Comment 3

[Richard Newman [:rnewman]]

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!

Comment 4

[Roberto Agostino Vitillo (:rvitillo)]

(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 :)

Comment 5

[Roberto Agostino Vitillo (:rvitillo)]

See bug 1007723.

Comment 6

[Richard Newman [:rnewman]]

(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!

Comment 7

[Mike Conley (:mconley) (:⚙️)]

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. :)

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

And 60KB is the file size, right? IME it takes up more than that once it's parsed into memory.

Comment 9

[Vladan Djeric (:vladan)]

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?

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: bug1041616-overdue-pings.zip

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.

Comment 12

[Richard Newman [:rnewman]]

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!

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 15

[Georg Fritzsche [:gfritzsche]]

vladan, you were working on a patch for bug 1126986, which was marked as a dupe of this.
Should this be assigned to you?

Comment 16

[Georg Fritzsche [:gfritzsche]]

Vladan was working on a patch in bug 1126986.

Comment 17

[Georg Fritzsche [:gfritzsche]]

Vladan has this WIP.

Comment 18

[Georg Fritzsche [:gfritzsche]]

Bug 1156355 tracks consolidating ping file load logic etc. in TelemetryStorage, which might have some overlap with this bug.

Comment 19

[Georg Fritzsche [:gfritzsche]]

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?

Comment 20

[Georg Fritzsche [:gfritzsche]]

(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.