Closed Bug 1482089 Opened 2 years ago Closed 2 months ago

Remove GCTelemetry to reduce memory usage in content process

Categories

(Toolkit :: Telemetry, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla80
Fission Milestone M6b
Tracking Status
firefox80 --- fixed

People

(Reporter: janerik, Assigned: pbone)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(4 files)

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
Blocks: 1475556
Priority: -- → P3
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)
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?
Flags: needinfo?(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.
Flags: needinfo?(pbone) → needinfo?(jrediger)
: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
Flags: needinfo?(jrediger) → needinfo?(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.
Assignee: nobody → pbone
Status: NEW → ASSIGNED
Flags: needinfo?(pbone)
Whiteboard: [MemShrink]
(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.
Whiteboard: [MemShrink] → [MemShrink:P2]
Priority: P3 → P2
See Also: → 1520357

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.

Type: defect → enhancement
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Refactor GCTelemetry to reduce memory usage in content process → Remove GCTelemetry to reduce memory usage in content process

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

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.

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

Depends on D84163

Also remove JSONUse since only one option is used now.

Depends on D84164

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

Fission Milestone: --- → M6b
Blocks: 1654155
Pushed by pbone@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.