-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Performance] Add minimal performance test to detect telemetry eviction #7312
[Performance] Add minimal performance test to detect telemetry eviction #7312
Comments
Getting 7 memory leaks, though none look related to imagery telemetry:
|
Running against YAMCS yield 4 unrelated memory leaks:
|
Last state of this is we've got a test in the openmct-yamcs repo, and it works if the telemetry is being emitted at ~100Hz. Below that, memlab doesn't seem to see that unbounded object growth of the plot series. The thing I'd like to add next is to get a trace of the object, find if they're openmct objects, and collect only those for the test instead of relying on hardcoded object types. |
Some open questions @akhenry @unlikelyzero @ozyx
|
note i’ve also synthetically tested this approach by just adding a quick return in the PlotSeries’s purge method. the test correctly identifies that telemetry is stacking up. |
Approach after conferring with @akhenry:
|
After talking with @akhenry, putting this one on hold for a bit. Current status:
The reason we're putting this on hold is that the telemetry objects themselves, at least within the Plot View, are almost immediately copied into a new array.. This means the telemetry objects are garbage collected, but also reduces the utility of this test. We should consider:
|
Digging into this more, I think I'm wrong about this. I did a bunch of testing in this branch adding finalization registers inside of the Telemetry API and the actual PlotSeries, and found the data being garbage collected at the same rate, and the same number of items: Screen.Recording.2024-02-16.at.12.02.44.PM.movSo I think this PR is probably fine as is. |
Summary
The text was updated successfully, but these errors were encountered: