Skip to content

Commit

Permalink
[1 of 2] Fix & re-enable perf profiler test: update Java test program…
Browse files Browse the repository at this point in the history
… to better match test expectations. (pixie-io#904)

Summary: In preparation for re-enabling the profiler test cases, we
update the Java test program such that its leaf functions do the exact
amount of work expected. Previously, we computed two different Fibonacci
numbers F27 and F52 with the expectation that F52 would require 2x the
work. Unfortunately, there was some variance between JVMs and test runs,
and this caused test flakiness.

In this patch, we simplify the test program. The new (and renamed) leaf
functions simply count by some increment. The increment is chosen such
that a certain leaf function does twice the counting and hence twice the
work.

In a future diff, to reduce test run time, we can remove some of the
Java test cases in a different PR.

Relevant Issues: pixie-io#719

Type of change: /kind bug fix.

Test Plan: Tested locally using --runs_per_test=32. All tests passed.
Also verified test flakiness by running tests without the fix.

---------

Signed-off-by: Pete Stevenson <jps@pixielabs.ai>
  • Loading branch information
Pete Stevenson authored Feb 27, 2023
1 parent ef16799 commit f510a50
Show file tree
Hide file tree
Showing 12 changed files with 150 additions and 129 deletions.
2 changes: 1 addition & 1 deletion src/stirling/source_connectors/perf_profiler/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ pl_cc_test(
px_jattach,
"//src/stirling/source_connectors/perf_profiler/testing/cc:profiler_test_app_fib",
"//src/stirling/source_connectors/perf_profiler/testing/go:profiler_test_app_sqrt_go",
"//src/stirling/source_connectors/perf_profiler/testing/java:graal-vm-aot-fib",
"//src/stirling/source_connectors/perf_profiler/testing/java:graal-vm-aot-test",
],
defines = ['JDK_IMAGE_NAMES=\\"%s\\"' % ",".join(jdk_image_names)],
shard_count = 3,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ pl_cc_test(
name = "attach_test",
srcs = ["attach_test.cc"],
args = stirling_profiler_attach_args,
data = ["//src/stirling/source_connectors/perf_profiler/testing/java:fib"] + agent_libs + [px_jattach],
data = ["//src/stirling/source_connectors/perf_profiler/testing/java:profiler_test"] + agent_libs + [px_jattach],
tags = [
"no_asan",
"requires_root",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ pl_cc_test(
],
data = [
":agent",
"//src/stirling/source_connectors/perf_profiler/testing/java:fib_with_agent",
"//src/stirling/source_connectors/perf_profiler/testing/java:profiler_test_with_agent",
],
tags = [
"exclusive",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ using ::px::testing::BazelRunfilePath;
using ::testing::HasSubstr;

TEST(JavaAgentTest, ExpectedSymbolsTest) {
constexpr std::string_view kJavaAppName = "fib_with_agent";
constexpr std::string_view kJavaAppName = "profiler_test_with_agent";

using fs_path = std::filesystem::path;
const fs_path kPathToJavaTesting = "src/stirling/source_connectors/perf_profiler/testing/java";
Expand Down Expand Up @@ -63,13 +63,13 @@ TEST(JavaAgentTest, ExpectedSymbolsTest) {

const absl::flat_hash_set<std::string> expected_symbols = {
"()J",
"fibs1x",
"fibs2x",
"leaf1x",
"leaf2x",
"([B[B)Z",
"LJavaFib;",
"call_stub",
"Interpreter",
"vtable stub",
"LProfilerTest;",
"Ljava/lang/Math;",
"()Ljava/lang/String;",
"(Ljava/lang/Object;)I",
Expand Down
10 changes: 5 additions & 5 deletions src/stirling/source_connectors/perf_profiler/java/attach_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ StatusOr<std::string> GetAbsPathLibsArg() {
} // namespace

// This test does the following:
// 1. Starts a target Java process (the fib app).
// 1. Starts a target Java process (the profiler test app).
// 2. Uses the AgentAttach class to inject a JVMTI agent (our symbolization agent).
// 3. Finds the resulting symbol file and verifies that it has some symbols.
// Before doing any of the above, we setup some file paths for the target app,
Expand All @@ -66,7 +66,7 @@ TEST(JavaAttachTest, ExpectedSymbolsTest) {
// Form the file name w/ user login to make it pedantically unique.
// Also, this is the same as in agent_test, so we keep the test logic consistent.

constexpr std::string_view kJavaAppName = "fib";
constexpr std::string_view kJavaAppName = "profiler_test";

using fs_path = std::filesystem::path;
const fs_path java_testing_path = "src/stirling/source_connectors/perf_profiler/testing/java";
Expand Down Expand Up @@ -131,13 +131,13 @@ TEST(JavaAttachTest, ExpectedSymbolsTest) {
// Check to see if the symbol file has some symbols.
const absl::flat_hash_set<std::string> expected_symbols = {
"()J",
"fibs1x",
"fibs2x",
"leaf1x",
"leaf2x",
"([B[B)Z",
"LJavaFib;",
"call_stub",
"Interpreter",
"vtable stub",
"LProfilerTest;",
"Ljava/lang/Math;",
"()Ljava/lang/String;",
"(Ljava/lang/Object;)I",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -454,14 +454,14 @@ TEST_F(PerfProfileBPFTest, PerfProfilerCppTest) {

// TODO(jps/oazizi): This test is flaky.
TEST_F(PerfProfileBPFTest, DISABLED_GraalVM_AOT_Test) {
const std::string app_path = "JavaFib";
const std::string app_path = "ProfilerTest";
const std::filesystem::path bazel_app_path = BazelJavaTestAppPath(app_path);
ASSERT_TRUE(fs::Exists(bazel_app_path)) << absl::StrFormat("Missing: %s.", bazel_app_path);

// The target app is written such that key2x uses twice the CPU time as key1x.
// For Java, we will match only the leaf symbol because we cannot predict the full stack trace.
constexpr std::string_view key2x = "JavaFib_fibs2x_f162aa2400fb352efe4ef0783a97b3b7ea0a389c";
constexpr std::string_view key1x = "JavaFib_fibs1x_fe3e196dc35a74f6bb512ebf4d440d375075fc93";
constexpr std::string_view key2x = "ProfilerTest_leaf2x_2971a14bad627821bd5c46dbdf969a8ab42430f5";
constexpr std::string_view key1x = "ProfilerTest_leaf1x_41af06c0834431228b8c265075a583c347b33636";

// Start target apps & create the connector context using the sub-process upids.
sub_processes_ = std::make_unique<CPUPinnedSubProcesses>(bazel_app_path);
Expand All @@ -485,8 +485,8 @@ TEST_P(PerfProfileBPFTest, PerfProfilerJavaTest) {

// The target app is written such that key2x uses twice the CPU time as key1x.
// For Java, we will match only the leaf symbol because we cannot predict the full stack trace.
constexpr std::string_view key2x = "[j] long JavaFib::fibs2x()";
constexpr std::string_view key1x = "[j] long JavaFib::fibs1x()";
constexpr std::string_view key2x = "[j] long ProfilerTest::leaf2x()";
constexpr std::string_view key1x = "[j] long ProfilerTest::leaf1x()";

// Start target apps & create the connector context using the sub-process upids.
sub_processes_ = std::make_unique<ContainerSubProcesses>(image_tar_path, kContainerNamePfx);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ pl_cc_test(
"--java_image_name $(location %s)" % java_image_tar,
],
data = [
"//src/stirling/source_connectors/perf_profiler/testing/java:fib",
"//src/stirling/source_connectors/perf_profiler/testing/java:profiler_test",
"//src/stirling/source_connectors/perf_profiler/testing/java:java",
] + [java_image_tar] + agent_libs + [px_jattach],
tags = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ TEST_F(BCCSymbolizerTest, DisableJavaSymbols) {
JavaSymbolizer::Create(std::move(symbolizer_)));

const std::filesystem::path java_app_path =
BazelRunfilePath("src/stirling/source_connectors/perf_profiler/testing/java/fib");
BazelRunfilePath("src/stirling/source_connectors/perf_profiler/testing/java/profiler_test");
ASSERT_TRUE(fs::Exists(java_app_path)) << java_app_path.string();
ASSERT_TRUE(fs::Exists(FLAGS_stirling_profiler_px_jattach_path))
<< FLAGS_stirling_profiler_px_jattach_path;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ load("//src/stirling/source_connectors/perf_profiler/testing:testing.bzl", "jdk_

package(default_visibility = ["//src/stirling:__subpackages__"])

# When invoking "fib_with_agent", to inject the JVMTI agent,
# When invoking "profiler_test_with_agent", to inject the JVMTI agent,
# we need a command line arg. that looks like this:
# -agentpath:/path/to/libagent.so=<symbolization-file-path-prefix>
# We build this up using so_path, symbol_file_path, and jvm_flags, below.
Expand All @@ -40,19 +40,19 @@ symbol_file_path = "./java-agent-test"
jvm_flags = [so_path + "=" + symbol_file_path]

java_binary(
name = "fib_with_agent",
srcs = ["Fib.java"],
name = "profiler_test_with_agent",
srcs = ["ProfilerTest.java"],
args = ["-XX:+PreserveFramePointer"],
data = ["//src/stirling/source_connectors/perf_profiler/java/agent"],
jvm_flags = jvm_flags,
main_class = "JavaFib",
main_class = "ProfilerTest",
)

java_binary(
name = "fib",
srcs = ["Fib.java"],
name = "profiler_test",
srcs = ["ProfilerTest.java"],
args = ["-XX:+PreserveFramePointer"],
main_class = "JavaFib",
main_class = "ProfilerTest",
)

pl_cc_binary(
Expand Down Expand Up @@ -94,43 +94,43 @@ jdk_symlinks = [symlinks_by_jdk[jdk_name] if jdk_name in symlinks_by_jdk else {}
[
java_image(
name = jdk_name + "-java-profiler-test-image",
srcs = ["Fib.java"],
srcs = ["ProfilerTest.java"],
args = ["-XX:+PreserveFramePointer"],
base = jdk_name,
main_class = "JavaFib",
main_class = "ProfilerTest",
)
for jdk_name in jdk_names
]

java_graal_binary(
name = "graal_java_fib",
srcs = ["Fib.java"],
name = "graal_java_profiler_test",
srcs = ["ProfilerTest.java"],
args = ["-XX:+PreserveFramePointer"],
main_class = "JavaFib",
main_class = "ProfilerTest",
)

graal_native_binary(
name = "graal_java_fib_aot",
name = "graal_java_test_aot",
extra_args = [
"--no-fallback",
"-H:Class=JavaFib",
"-H:Class=ProfilerTest",
"-H:-DeleteLocalSymbols",
"-H:+PreserveFramePointer",
"-H:+ReportUnsupportedElementsAtRuntime",
],
graal_runtime = "@remotejdk_openjdk_graal_17//:jdk",
java_binary = ":graal_java_fib",
output_name = "JavaFib",
java_binary = ":graal_java_profiler_test",
output_name = "ProfilerTest",
)

filegroup(
name = "graal-vm-aot-fib",
srcs = [":graal_java_fib_aot"],
name = "graal-vm-aot-test",
srcs = [":graal_java_test_aot"],
)

java_image(
name = "java_image_base-java-profiler-test-image-omit-frame-pointer",
srcs = ["Fib.java"],
srcs = ["ProfilerTest.java"],
base = "java_image_base",
main_class = "JavaFib",
main_class = "ProfilerTest",
)
88 changes: 0 additions & 88 deletions src/stirling/source_connectors/perf_profiler/testing/java/Fib.java

This file was deleted.

Loading

0 comments on commit f510a50

Please sign in to comment.