Closed Bug 1323382 Opened 7 years ago Closed 7 years ago

static FFmpegLibWrapper constructors

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set
normal

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 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+
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.
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
https://hg.mozilla.org/mozilla-central/rev/769c39de5f3e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
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.

Attachment

General

Created:
Updated:
Size: