Skip to content
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

Improve TCMalloc detection #443

Merged
merged 3 commits into from
Jun 1, 2021

Conversation

mediaminister
Copy link
Collaborator

@mediaminister mediaminister commented May 27, 2021

This checks if TCMalloc is preloaded or linked using /proc/self/maps

@mediaminister mediaminister added enhancement arm Related to ARM architecture labels May 27, 2021
@mediaminister mediaminister added this to the v0.5.5 milestone May 27, 2021
@dagwieers
Copy link
Collaborator

So I wasn't sure if all distributions are using the exact same implementation. But if preloading is the only way to make it work, and it is done when Kodi starts, it means our add-on has libtcmalloc preloaded as well.

So I was wondering within python if we can detect whether libtcmalloc was loaded, or whether we can detect that from our process memory (i.e. /proc/pid).

It would make the implementation somewhat more robust (if my assumptions are correct).

@mediaminister
Copy link
Collaborator Author

mediaminister commented May 28, 2021

Maybe this is possible from command line with lsof /usr/lib/libtcmalloc_minimal.so (using subprocess in python)

EDIT: works on Linux x64 but not in LibreELEC

@mediaminister
Copy link
Collaborator Author

mediaminister commented May 28, 2021

I think I found a solution based on these commands:

pidof kodi.bin
cat /proc/{pid}/maps

Test packages to use with the nightly builds of LibreELEC and CoreELEC:
script.module.inputstreamhelper-0.5.4+matrix.1-tcmalloc-248f01d.zip
script.module.inputstreamhelper-0.5.4-tcmalloc-248f01d.zip

@mediaminister mediaminister marked this pull request as ready for review May 28, 2021 17:14
@mediaminister mediaminister requested a review from dagwieers May 28, 2021 17:15
@dagwieers
Copy link
Collaborator

dagwieers commented May 30, 2021

So I think this can be simplified by doing:

content = open('/proc/self/maps', 'r').read()

You do not have to call a unix command to read a file. And we can use the python child process to find if the library is preloaded. The implementation would something like:

    libtcmalloc = 'libtcmalloc_minimal.so'
    process_maps = open('/proc/self/maps', 'r').read()
    is_tcmalloc_preloaded = bool(libtcmalloc in process_maps)

    arm_device = None
    if not is_tcmalloc_preloaded:

We do not so much care about the exact location of libtcmalloc_minimal.so or even if the file exists.
For us it is sufficient to know it is loaded.
I am not even sure we care it is called libtcmalloc_minimal.so, so maybe we should look for just libtcmalloc ?

@dagwieers
Copy link
Collaborator

dagwieers commented May 30, 2021

What would be even better is if we can verify if the calls that Widevine is doing are available. In that case we do not even need to check for the library in the process maps, we could simply ask python if those symbols are available in its process space and conclude they are also available to libwidevinecdm.so.

Any of these would probably do:

tcmalloc.aggressive_memory_decommit
tcmalloc.central_cache_free_bytes
tcmalloc.current_total_thread_cache_bytes
tcmalloc.max_total_thread_cache_bytes
tcmalloc.pageheap_commit_count
tcmalloc.pageheap_committed_bytes
tcmalloc.pageheap_decommit_count
tcmalloc.pageheap_free_bytes
tcmalloc.pageheap_reserve_count
tcmalloc.pageheap_scavenge_count
tcmalloc.pageheap_total_commit_bytes
tcmalloc.pageheap_total_decommit_bytes
tcmalloc.pageheap_total_reserve_bytes
tcmalloc.pageheap_unmapped_bytes
tcmalloc.sampling_period_bytes
tcmalloc.slack_bytes
tcmalloc.thread_cache_free_bytes
tcmalloc.transfer_cache_free_bytes

With ctypes you can easily load a library and then check if any of these symbols are present (and even call them). But I cannot easily find a way to access already loaded (preloaded or linked to python interpreter itself). libraries. It must be possible though.

@dagwieers
Copy link
Collaborator

But we can make those improvements later as well. It is more important that we have something that works well enough on all supported platforms. Whatever works the broadest is best.

@mediaminister
Copy link
Collaborator Author

Thanks, I simplified the check.

@mediaminister mediaminister marked this pull request as draft May 31, 2021 15:24
@mediaminister mediaminister modified the milestones: v0.5.5, Future May 31, 2021
@dagwieers
Copy link
Collaborator

dagwieers commented May 31, 2021

So, if CoreELEC is linking to libtcmalloc.so or libtcmalloc_minimal.so and it is not preloaded, I assumed that we could no longer check python's process memory for tcmalloc symbols, and this would be a dead-end. I also assumed that using /proc/self/maps will effectively no longer work. But that is not true.

I expected that the parent pid (ppid) of python would be the Kodi binary (kodi.bin) but that is not the case. On LibreELEC the parent of the python interpreter is in fact kodi.sh (the startup wrapper). So the python interpreter is in fact compiled into kodi.bin (big surprise).

In fact that is not a big surprise, because that's how it can cache previous runs of an add-on. And we already established that add-ons were being runs as LanguageInvoker. So here you go, the current implementation works on CoreELEC just fine. We did a good job, well done ! 😆

lib/inputstreamhelper/widevine/arm.py Outdated Show resolved Hide resolved
lib/inputstreamhelper/widevine/arm.py Outdated Show resolved Hide resolved
mediaminister and others added 2 commits June 1, 2021 11:17
Co-authored-by: Dag Wieers <dag@wieers.com>
Co-authored-by: Dag Wieers <dag@wieers.com>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 1, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@dagwieers
Copy link
Collaborator

Do we know who can test this on CoreELEC for us?

@mediaminister
Copy link
Collaborator Author

I think @CastagnaIT tested the CoreELEC nightly before.

@CastagnaIT
Copy link

CastagnaIT commented Jun 1, 2021

i have tested this PR on last coreelec nightly by choosing "Re-install widevine cdm lib" setting menu
and have reinstalled the cdm without asking to install the old wv library
then i think this works good

@mediaminister mediaminister marked this pull request as ready for review June 1, 2021 18:40
@mediaminister
Copy link
Collaborator Author

Thanks for confirming.

@mediaminister mediaminister merged commit 6edf690 into emilsvennesson:master Jun 1, 2021
@mediaminister mediaminister modified the milestones: Future, v0.5.5 Jun 1, 2021
@Portisch
Copy link

Portisch commented Jun 2, 2021

Thank you for taking care about the linked in option 👍

We currently see some users reporting rebooting devices and we maybe switch to the LD_PRELOAD method as it looks "more stable" and causes less rebooting devices.

Currently we trigger two errors with the new cdm lib: rebooting while watching, like after 20-60min and a crash when a stream is opened/closed multiple times.

@CastagnaIT
Copy link

@Portisch you are talking about coreelec or libreelec?
you should report the error crash log to xbmc/inputstream.adaptive#678

@Portisch
Copy link

Portisch commented Jun 2, 2021

CoreELEC only 😉

First users reporting already tcmalloc LD_PRELOAD works "better" than direct linking and does not reboot the device anymore.
Only a handfull of users are affected, seems the users with a bunch of addon installed.

For the other issue when multiple times open/close a stream I will report when I collected some more debug info, thank you!

@CastagnaIT
Copy link

Tomorrow i will try to play multiple times with my coreelec to see if happen also to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arm Related to ARM architecture enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants