static FFmpegLibWrapper constructors

RESOLVED FIXED in Firefox 53

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

({regression})

Trunk
mozilla53
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 unaffected, firefox51 unaffected, firefox52 unaffected, firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)
Comment hidden (mozreview-request)

Comment 2

10 months 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

10 months 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

10 months 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"}]}

Comment 5

10 months ago
Pushed by ktomlinson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/769c39de5f3e
remove static FFmpegLibWrapper constructor and destructors r=jya

Updated

10 months ago
status-firefox50: --- → unaffected
status-firefox51: --- → unaffected
status-firefox52: --- → unaffected

Comment 6

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/769c39de5f3e
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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.