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

[Ready-to-review] Adding caching to PyPerfNativeStackTrace #421

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

IzabellaRaulin
Copy link
Contributor

@IzabellaRaulin IzabellaRaulin commented Jul 25, 2022

Resolving the issue #379

  • in-progress: Updating the performance results for the latest commit

1. Summary of changes

2. Results [this section will be updated soon]

Methodology:

  • 10 python applications (doing while 1: pass) were running for all this time
  • Running dockerized gprofiler in DWARF mode and set profiling frequency to 99 Hz, see command below:
    docker run --network host --name <name> --restart=always -d --pid=host --userns=host -v 
    /var/run/docker.sock:/var/run/docker.sock --privileged <my-image-name:tag> -cu --token=<my_token> --service-name= 
    <my_service_name>" -f 99 --perf-mode dwarf
    
  • Data uploaded to services (7th Oct' 22):
    • gprofiler-official-1.6.0 (collection_start: 5:50 PM CET, collection_end: 6:20 PM CET)
    • gprofiler-caching-revision-1.2 (collection_start: 6:30 PM CET, collection_end: 7:00 PM CET)
For granulate/gprofiler:1.6.0
_Ux86_64_get_proc_name Total time 3.52% Own time: 0.0% Samples: 19,700
ebpf::pyperf::NativeStackTrace::NativeStackTrace Total time 4.43% Own time: 0.0% Samples: 24,302
ebpf::pyperf::NativeStackTrace::cache_eviction N/A
For pyPerfNativeStack with enabled caching
_Ux86_64_get_proc_name Total time 3.15% Own time: 0.0% Samples: 15,108
ebpf::pyperf::NativeStackTrace::NativeStackTrace Total time 4.1% Own time: 0.0% Samples: 21,357
ebpf::pyperf::NativeStackTrace::cache_eviction Total time 0.13% Own time: 0.% Samples: 727

3. Screenshot for the scenario with official docker image (granulate/gprofiler:1.6.0)

3.1. _Ux86_64_get_proc_name matches
image
3.2. _Ux86_64_get_proc_name matches for filtered Pyperf process
image

4. Screenshots for the scenario with enabled caching in PyPerfNativeStack

4.1. _Ux86_64_get_proc_name matches
image
4.2. _Ux86_64_get_proc_name matches for filtered Pyperf process
image
4.3. cache_exiction matches
image
4.4. cache_exiction matches for filtered Pyperf process
image

5. Comparison

Before (docker image granulate/gprofiler:1.6.0):
image

After enabling caching
image

@IzabellaRaulin IzabellaRaulin changed the title [DO NOT MERGE][WIP] Adding caching to PyPerfNativeStackTrace Adding caching to PyPerfNativeStackTrace Sep 13, 2022
@IzabellaRaulin IzabellaRaulin changed the title Adding caching to PyPerfNativeStackTrace [Ready-to-review] Adding caching to PyPerfNativeStackTrace Sep 13, 2022
@Jongy
Copy link
Contributor

Jongy commented Sep 18, 2022

@IzabellaRaulin , the comparison methology looks good.

Going forward - I put a few more comments on the BCC PR. Then we can merge it.
Before we merge this PR, I'll get #478 merged so that PyPerf & native unwinding is tested again against our full matrix. After thart we're good to go :) The Python tests matrix PR will be merged this week.

@Jongy
Copy link
Contributor

Jongy commented Sep 22, 2022

@IzabellaRaulin please merge from master as I have merged #478 which adds more tests for PyPerf, I want your changes to be tested by them as well.

Call pruning dead pids from the cache in populatePidsTable() + fix bug with missing native symbols
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants