Closed
Bug 1323382
Opened 7 years ago
Closed 7 years ago
static FFmpegLibWrapper constructors
Categories
(Core :: Audio/Video: Playback, defect)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox50 | --- | unaffected |
firefox51 | --- | unaffected |
firefox52 | --- | unaffected |
firefox53 | --- | fixed |
People
(Reporter: karlt, Assigned: karlt)
References
Details
(Keywords: regression)
Attachments
(1 file)
No description provided.
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8818475 [details] bug 1323382 remove static FFmpegLibWrapper constructor and destructors https://reviewboard.mozilla.org/r/98538/#review98794 I think it would have been more elegant to change in FFMpegRunTimeLinker: static FFmpegLibWrapper sLibAV; into static FFmpegLibWrapper* sLibAV = nullptr and create a new one on the stack in FFmpegRuntimeLinker::Init() There's no need to worry about data races as this is only called by the PDMFactory initialisation which is thread safe and only ever called once. Then register that point with ClearOnShutdown, so it will be deleted during shutdown. But I guess leaking on shutdown doesn't matter here.
Attachment #8818475 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 3•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8818475 [details] bug 1323382 remove static FFmpegLibWrapper constructor and destructors https://reviewboard.mozilla.org/r/98538/#review98794 FFmpegLibWrapper could be created on the heap (I assume you mean heap rather than stack), and ClearOnShutdown() could be used with StaticAutoPtr<FFmpegLibWrapper>, if closing the library were desired, but I would like to avoid closing the library to suppress the leak in bug 1304156. If the library is not closed, then the heap allocation and ClearOnShutdown() are just unnecessary code. There was a time when Gecko aimed to close dlopened libraries at shutdown for the sake of memory leak tools that didn't check for static references. LSAN checks for static references and so there is little benefit from closing the library at shutdown.
Assignee | ||
Comment 4•7 years ago
|
||
before patch: % python build/util/count_ctors.py obj-opt/dist/bin/libxul.so PERFHERDER_DATA: {"framework": {"name": "build_metrics"}, "suites": [{"subtests": [{"name": "num_constructors", "value": 101, "alertThreshold": 0.25}], "name": "compiler_metrics"}]} after patch: % python build/util/count_ctors.py obj-opt/dist/bin/libxul.so PERFHERDER_DATA: {"framework": {"name": "build_metrics"}, "suites": [{"subtests": [{"name": "num_constructors", "value": 99, "alertThreshold": 0.25}], "name": "compiler_metrics"}]}
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/769c39de5f3e remove static FFmpegLibWrapper constructor and destructors r=jya
Updated•7 years ago
|
status-firefox50:
--- → unaffected
status-firefox51:
--- → unaffected
status-firefox52:
--- → unaffected
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/769c39de5f3e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 7•7 years ago
|
||
thanks for landing this, our measurement of num_constructors showed an improvement: == Change summary for alert #4515 (as of December 15 2016 01:55 UTC) == Improvements: 2% compiler_metrics num_constructors linux32 pgo 100 -> 98 2% compiler_metrics num_constructors linux64 pgo 100 -> 98 2% compiler_metrics num_constructors linux32 opt 100 -> 98 1% compiler_metrics num_constructors linux32 debug 181 -> 179 1% compiler_metrics num_constructors linux64 debug 181 -> 179 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4515
You need to log in
before you can comment on or make changes to this bug.
Description
•