Skip to content

Conversation

@Jongy
Copy link
Contributor

@Jongy Jongy commented Jul 11, 2021

Description

By embedding the gProfiler version in AP dir, we will have different dirs for different gProfiler
releases.

Note: This breaks the "AP stop" logic across gProfiler versions: if I run version X, kill it brutally, then run version Y, the new AP will be loaded as a different DSO (since it has a different path) and it may cause harm (see async-profiler/async-profiler#395 for example). Need to think on a workaround / a different approach for allowing AP upgrades. Opening this PR for discussion.

Motivation and Context

Allow different gProfiler versions to use different APs. So if we upgrade the AP version in a future gProfiler release, and gProfiler is upgraded on a box which already ran an old version, it will use the new binary.

How Has This Been Tested?

  • Run a Java process; Run gProfiler; Change something in AP, build gProfiler (with a different __version__) and run again and ensure the new one is used.

Checklist:

  • I have read the CONTRIBUTING document.
  • I have updated the relevant documentation.
  • I have added tests for new logic.

By embedding the gProfiler version in AP dir, we will have different dirs for different gProfiler
releases.
@Jongy Jongy added the enhancement New feature or request label Jul 11, 2021
@Jongy Jongy requested a review from liorgorb July 11, 2021 23:24
@Jongy
Copy link
Contributor Author

Jongy commented Jul 12, 2021

CI failures - #138

@Jongy Jongy marked this pull request as ready for review July 12, 2021 20:56
@Jongy
Copy link
Contributor Author

Jongy commented Jul 12, 2021

Note: This breaks the "AP stop" logic across gProfiler versions: if I run version X, kill it brutally, then run version Y, the new AP will be loaded as a different DSO (since it has a different path) and it may cause harm (see async-profiler/async-profiler#395 for example). Need to think on a workaround / a different approach for allowing AP upgrades. Opening this PR for discussion.

This issue remains open. I don't have a simple workaround for it. I tried to minimize its chances by embedding AP's version in the path (instead of gProfiler's) under the assumption that we release AP versions less often than we release gProfiler versions.

So we break the "AP stop" logic across AP upgrades. Fair enough. Not very typical.

@Jongy
Copy link
Contributor Author

Jongy commented Jul 12, 2021

Tested - I ran gProfiler from this branch, then added this diff:

diff --git a/scripts/async_profiler_build.sh b/scripts/async_profiler_build.sh
index cb8e09f..dc8294b 100755
--- a/scripts/async_profiler_build.sh
+++ b/scripts/async_profiler_build.sh
@@ -5,10 +5,10 @@
 #
 set -euo pipefail
 
-VERSION=v2.0g3
+VERSION=v2.0g2
 OUTPUT=async-profiler-2.0-linux-x64.tar.gz
 
-git clone --depth 1 -b "$VERSION" https://github.com/Granulate/async-profiler.git && cd async-profiler && git reset --hard 51447a849d686e899c1cd393e83f0f7c41685d95
+git clone --depth 1 -b "$VERSION" https://github.com/Granulate/async-profiler.git && cd async-profiler && git reset --hard 9692c57ab9b3f77cd489a6ee26cad18d081c3e45
 make release
 
 # add a version file to the build directory

and built again.

We can see 2 APs were loaded:

$ grep libasync /proc/876125/maps
7f03019a1000-7f03019e1000 r-xp 00000000 fd:01 35688784                   /tmp/gprofiler_tmp/async-profiler-v2.0g2/libasyncProfiler.so
7f03019e1000-7f0301be1000 ---p 00040000 fd:01 35688784                   /tmp/gprofiler_tmp/async-profiler-v2.0g2/libasyncProfiler.so
7f0301be1000-7f0301be3000 rw-p 00040000 fd:01 35688784                   /tmp/gprofiler_tmp/async-profiler-v2.0g2/libasyncProfiler.so
7f03025f0000-7f0302630000 r-xp 00000000 fd:01 35688758                   /tmp/gprofiler_tmp/async-profiler-v2.0g3/libasyncProfiler.so
7f0302630000-7f0302830000 ---p 00040000 fd:01 35688758                   /tmp/gprofiler_tmp/async-profiler-v2.0g3/libasyncProfiler.so
7f0302830000-7f0302832000 rw-p 00040000 fd:01 35688758                   /tmp/gprofiler_tmp/async-profiler-v2.0g3/libasyncProfiler.so

@Jongy Jongy merged commit 989934b into master Jul 13, 2021
@Jongy Jongy deleted the java-async-profiler-per-version branch July 13, 2021 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants