1482089 - Remove GCTelemetry to reduce memory usage in content process

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P2)

Description

[Jan-Erik Rediger [:janerik]]

This needs a bit more investigation, but a first PoC[1] extracted functionality into a content-only module.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfaa066f125b2412dbf8feaf3f30c7316cb87217

Comment 1

[Jan-Erik Rediger [:janerik]]

In bug 1481812 we move GCTelemetry into MemoryTelemetry. We should check if we can instead move it into the TelemetryController (we should be careful to avoid loading it during early startup though)

Comment 2

[Jan-Erik Rediger [:janerik]]

Hey :pbone,

you previously did some work on GCTelemetry.jsm.
GCTelemetry.jsm is one of the things that remains in the content process right now. One way to reduce that is rewriting it in C++, which will probably use less memory right away.

Do you think this is realistic? Would you be the right person to tackle this or knows who would be able to do it?

Comment 3

[Paul Bone [:pbone]]

I think it could be rewritten in C++ yes.  Do you have an example of some other telemetry code that is n C++, then I could use that to work out what calls it needs to make/answer and what information to provide.

Comment 4

[Jan-Erik Rediger [:janerik]]

:kmag recently rewrote our TelemetryStopwatch utility into WebIDL, implemented in C++ (bug 1482091).
As GCTelemetry doesn't need to interact with other Telemetry components directly, I think that would be the way to go.
Currently the GC data is already converted to JSON[1] and send through the notification system.
Parsing that in C++ might be a hassle, but maybe there are other ways to get to the data in a easier-to-work-with format?

On the Telemetry side we just need the format out as an object again to add it to the ping[2]
 
[1]: https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/dom/base/nsJSEnvironment.cpp#2352
[2]: https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/toolkit/components/telemetry/pings/TelemetrySession.jsm#986-987

Comment 5

[Paul Bone [:pbone]]

I would prefer to keep/put the data in C++ structures/classes and then when we choose which data to send (because GCTelemetry.jsm needs to be selective) we render it to JSON.  This will also save memory.

Comment 6

[Paul Bone [:pbone]]

(In reply to Jan-Erik Rediger [:janerik] from comment #4)
> :kmag recently rewrote our TelemetryStopwatch utility into WebIDL,
> implemented in C++ (bug 1482091).
> As GCTelemetry doesn't need to interact with other Telemetry components
> directly, I think that would be the way to go.
> Currently the GC data is already converted to JSON[1] and send through the
> notification system.
> Parsing that in C++ might be a hassle, but maybe there are other ways to get
> to the data in a easier-to-work-with format?
> 
> On the Telemetry side we just need the format out as an object again to add
> it to the ping[2]
>  
> [1]:
> https://searchfox.org/mozilla-central/rev/
> b03a62c3c82316e733a3b09622c1cb7e59f64cc3/dom/base/nsJSEnvironment.cpp#2352
> [2]:
> https://searchfox.org/mozilla-central/rev/
> b03a62c3c82316e733a3b09622c1cb7e59f64cc3/toolkit/components/telemetry/pings/
> TelemetrySession.jsm#986-987

Thank you for the info.

Comment 7

[Paul Bone [:pbone]]

The original bug asked for this telemetry code to be refactored (moved to C++) so it could consume less memory. But I don't think anyone is using it so I propose to remove it.

Comment 8

[Paul Bone [:pbone]]

(In reply to Paul Bone [:pbone] from comment #7)

The original bug asked for this telemetry code to be refactored (moved to C++) so it could consume less memory. But I don't think anyone is using it so I propose to remove it.

I want to clarify. This is not the GC Telemetry scalars that are used often, but the JSON/JS objects describing some typical & worst GC events.

Comment 9

[Paul Bone [:pbone]]

Attachment: Bug 1482089 - pt 1. Remove DOM code for processing GCTelemetry r=mccr8

This code glues the JS GC code that creates the telemetry with a JS module
that processes it. This patch removes this code unhooking these
components.

Comment 10

[Paul Bone [:pbone]]

Attachment: Bug 1482089 - pt 2. Remove GCTelemetry modules and tests r=chutten

• Remove the GCTelemetry module
• Remove the TelemetryGC tests
• Don't attempt to send the GC telemetry data
• Update the telemetry docs

Depends on D84163

Comment 11

[Paul Bone [:pbone]]

Attachment: Bug 1482089 - pt 3. Remove formatJsonTelemetry from GCAPI r=jonco

Also remove JSONUse since only one option is used now.

Depends on D84164

Comment 12

[Paul Bone [:pbone]]

Attachment: Bug 1482089 - pt 4. Remove comments about GCTelemetry r=jonco

Since this JSON-formatting code was used for profiling and telemetry it
contained notes and numbered fields for both. Remove the field numbers and
the notes regarding GCTelemetry.

Also remove GCTelemetry references from GeneratestatePhases.py.

Depends on D84165

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d19174478191
pt 1. Remove DOM code for processing GCTelemetry r=mccr8
https://hg.mozilla.org/integration/autoland/rev/790c1fca5d56
pt 2. Remove GCTelemetry modules and tests r=chutten
https://hg.mozilla.org/integration/autoland/rev/31863f9a323b
pt 3. Remove formatJsonTelemetry from GCAPI r=jonco
https://hg.mozilla.org/integration/autoland/rev/8f47231420c5
pt 4. Remove comments about GCTelemetry r=jonco

Comment 14

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/d19174478191
https://hg.mozilla.org/mozilla-central/rev/790c1fca5d56
https://hg.mozilla.org/mozilla-central/rev/31863f9a323b
https://hg.mozilla.org/mozilla-central/rev/8f47231420c5