1323382 - static FFmpegLibWrapper constructors

Status: RESOLVED FIXED

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

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: bug 1323382 remove static FFmpegLibWrapper constructor and destructors

The libraries are no longer closed, which works around bug 1304156.

The MOZ_ONLY_USED_TO_AVOID_STATIC_CONSTRUCTORS name seems a little odd here
but it ensures the desired class behaviour and usage, and is consistent with
changes for bug 1226376.

Review commit: https://reviewboard.mozilla.org/r/98538/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98538/

Comment 2

[Jean-Yves Avenard [:jya]]

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.

Comment 3

[Karl Tomlinson (:karlt)]

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.

Comment 4

[Karl Tomlinson (:karlt)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/769c39de5f3e
remove static FFmpegLibWrapper constructor and destructors r=jya

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/769c39de5f3e

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

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