From b084e0e23535cb7e46802c3bf471ee8cafbe8a74 Mon Sep 17 00:00:00 2001 Subject: Fix a concurrent issue of program data ref cnt --- .../share/jbooster/jClientArguments.hpp | 4 + .../jbooster/server/serverDataManager.hpp | 4 + .../utilities/concurrentHashMap.inline.hpp | 20 +--- test/hotspot/gtest/jbooster/test_net.cpp | 1 + test/hotspot/gtest/jbooster/test_server.cpp | 107 ++++++++++++++++++ test/hotspot/gtest/jbooster/test_util.cpp | 3 +- .../jbooster/JBoosterConcurrentTest.java | 106 +++++++++++++++++ test/jdk/tools/jbooster/TEST.properties | 1 + 8 files changed, 231 insertions(+), 15 deletions(-) create mode 100644 test/hotspot/gtest/jbooster/test_server.cpp create mode 100644 test/jdk/tools/jbooster/JBoosterConcurrentTest.java create mode 100644 test/jdk/tools/jbooster/TEST.properties diff --git a/src/hotspot/share/jbooster/jClientArguments.hpp b/src/hotspot/share/jbooster/jClientArguments.hpp index 491ac3159..dd56ddc36 100644 --- a/src/hotspot/share/jbooster/jClientArguments.hpp +++ b/src/hotspot/share/jbooster/jClientArguments.hpp @@ -60,6 +60,9 @@ DECLARE_SERIALIZER_INTRUSIVE(JClientBoostLevel); * Arguments that identify a program. */ class JClientArguments final: public CHeapObj { + // @see test/hotspot/gtest/jbooster/test_server.cpp + friend class Gtest_JBoosterServer; + enum InitState { NOT_INITIALIZED_FOR_SEREVR, INITIALIZED_FOR_SEREVR, @@ -103,6 +106,7 @@ private: public: JClientArguments(bool is_client); + JClientArguments(DebugUtils* ignored) {} // for GTEST only ~JClientArguments(); int serialize(MessageBuffer& buf) const; diff --git a/src/hotspot/share/jbooster/server/serverDataManager.hpp b/src/hotspot/share/jbooster/server/serverDataManager.hpp index c6ef4c99a..a53af341c 100644 --- a/src/hotspot/share/jbooster/server/serverDataManager.hpp +++ b/src/hotspot/share/jbooster/server/serverDataManager.hpp @@ -173,6 +173,9 @@ public: * The data of the program (usually a .class or .jar file) executed by the client. */ class JClientProgramData final: public CHeapObj { + // @see test/hotspot/gtest/jbooster/test_server.cpp + friend class Gtest_JBoosterServer; + public: using ClassLoaderTable = ConcurrentHashMap; @@ -195,6 +198,7 @@ private: public: JClientProgramData(uint32_t program_id, JClientArguments* program_args); + JClientProgramData(DebugUtils* ignored) {} // for GTEST only ~JClientProgramData(); uint32_t program_id() const { return _program_id; } diff --git a/src/hotspot/share/jbooster/utilities/concurrentHashMap.inline.hpp b/src/hotspot/share/jbooster/utilities/concurrentHashMap.inline.hpp index e69da290f..8abb853b7 100644 --- a/src/hotspot/share/jbooster/utilities/concurrentHashMap.inline.hpp +++ b/src/hotspot/share/jbooster/utilities/concurrentHashMap.inline.hpp @@ -168,8 +168,8 @@ inline V* ConcurrentHashMap::get(const K& key, Thread* thread) V* res = nullptr; if (_table.get(thread, lookup, get, &grow_hint)) { - KVNode& kv_node = *get.res(); - res = &kv_node.value(); + assert(get.res() != nullptr, "sanity"); + res = &(get.res()->value()); } return res; @@ -184,18 +184,10 @@ inline V* ConcurrentHashMap::put_if_absent(const K& key, V& val V* res = nullptr; - do { - if (_table.insert(thread, lookup, KVNode(key, value), &grow_hint, &clean_hint)) { - res = &value; - break; - } - - if (_table.get(thread, lookup, get, &grow_hint)) { - KVNode& kv_node = *get.res(); - res = &kv_node.value(); - break; - } - } while (true); + KVNode kv_node(key, value); + bool success = _table.insert_get(thread, lookup, kv_node, get, &grow_hint, &clean_hint); + assert(get.res() != nullptr, "sanity"); + res = &(get.res()->value()); if (grow_hint) { grow_if_needed(thread); diff --git a/test/hotspot/gtest/jbooster/test_net.cpp b/test/hotspot/gtest/jbooster/test_net.cpp index dd45dd65a..336418cf4 100644 --- a/test/hotspot/gtest/jbooster/test_net.cpp +++ b/test/hotspot/gtest/jbooster/test_net.cpp @@ -37,6 +37,7 @@ #include "runtime/os.inline.hpp" #include "utilities/globalDefinitions.hpp" #include "utilities/growableArray.hpp" + #include "unittest.hpp" static int try_catch_func(int i) { diff --git a/test/hotspot/gtest/jbooster/test_server.cpp b/test/hotspot/gtest/jbooster/test_server.cpp new file mode 100644 index 000000000..322b947fd --- /dev/null +++ b/test/hotspot/gtest/jbooster/test_server.cpp @@ -0,0 +1,107 @@ +/* + * Copyright (c) 2020, 2024, Huawei Technologies Co., Ltd. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +#include "precompiled.hpp" +#include "utilities/macros.hpp" + +#if INCLUDE_JBOOSTER + +#include "common.hpp" + +#include "jbooster/jClientArguments.hpp" +#include "jbooster/server/serverDataManager.hpp" +#include "jbooster/utilities/concurrentHashMap.inline.hpp" +#include "jbooster/utilities/debugUtils.inline.hpp" +#include "utilities/stringUtils.hpp" + +#include "unittest.hpp" + +struct Gtest_JBoosterServer { + static JClientArguments* create_mock_JClientArguments(uint32_t id) { + JClientArguments* mock = new JClientArguments((DebugUtils*) nullptr); + + // [FOR EACH ARG] + mock->_cpu_arch = JClientArguments::CpuArch::CPU_UNKOWN; + mock->_jvm_version = id; + mock->_internal_jvm_info = StringUtils::copy_to_heap("internaljvminfo", mtJBooster); + mock->_program_name = StringUtils::copy_to_heap("programname", mtJBooster); + mock->_program_entry = StringUtils::copy_to_heap("programentry", mtJBooster); + mock->_is_jar = true; + mock->_classpath_name_hash = 1u; + mock->_classpath_timestamp_hash = 2u; + mock->_agent_name_hash = 3u; + mock->_cds_file_size = 456; + mock->_java_commands = StringUtils::copy_to_heap("javacommands", mtJBooster); + mock->_related_flags = new JClientVMFlags(true); + + mock->_hash = mock->calc_hash(); + mock->_state = JClientArguments::INITIALIZED_FOR_SEREVR; + + return mock; + } + + static JClientProgramData* create_mock_JClientProgramData(uint32_t program_id, JClientArguments* program_args) { + JClientProgramData* mock = new JClientProgramData((DebugUtils*) nullptr); + + mock->_program_id = program_id; + mock->_program_args = program_args->move(); + mock->_ref_cnt.inc(); + mock->_program_str_id = StringUtils::copy_to_heap("programstrid", mtJBooster); + mock->clr_cache_state().init(StringUtils::copy_to_heap("gtestclr", mtJBooster)); + mock->dy_cds_cache_state().init(StringUtils::copy_to_heap("gtestdycds", mtJBooster)); + mock->agg_cds_cache_state().init(StringUtils::copy_to_heap("gtestaggcds", mtJBooster)); + mock->aot_static_cache_state().init(StringUtils::copy_to_heap("gtestaotstatic", mtJBooster)); + mock->aot_pgo_cache_state().init(StringUtils::copy_to_heap("gtestaotpgo", mtJBooster)); + + return mock; + } +}; + +TEST_VM(JBoosterServer, program_data_ref_cnt) { + JavaThread* THREAD = JavaThread::current(); + ServerDataManager::JClientProgramDataMap programs; + JClientArguments* program_args1 = Gtest_JBoosterServer::create_mock_JClientArguments(11); + JClientProgramData* new_pd1 = Gtest_JBoosterServer::create_mock_JClientProgramData(1u, program_args1); + delete program_args1; + JClientProgramData** res1 = programs.put_if_absent(new_pd1->program_args(), new_pd1, THREAD); + EXPECT_NE(res1, (JClientProgramData**) nullptr); + EXPECT_EQ(*res1, new_pd1); + EXPECT_EQ(new_pd1->ref_cnt().get(), 1); + + JClientArguments* program_args2 = Gtest_JBoosterServer::create_mock_JClientArguments(11); + JClientProgramData* new_pd2 = Gtest_JBoosterServer::create_mock_JClientProgramData(2u, program_args2); + delete program_args2; + JClientProgramData** res2 = programs.put_if_absent(new_pd2->program_args(), new_pd2, THREAD); + EXPECT_NE(res2, (JClientProgramData**) nullptr); + EXPECT_EQ(*res2, new_pd1); + EXPECT_EQ(new_pd1->ref_cnt().get(), 2); + EXPECT_EQ(new_pd2->ref_cnt().get(), 1); + + new_pd2->ref_cnt().dec_and_update_time(); + delete new_pd2; + + new_pd1->ref_cnt().dec_and_update_time(); + new_pd1->ref_cnt().dec_and_update_time(); +} + +#endif // INCLUDE_JBOOSTER diff --git a/test/hotspot/gtest/jbooster/test_util.cpp b/test/hotspot/gtest/jbooster/test_util.cpp index 461e3faa7..178998189 100644 --- a/test/hotspot/gtest/jbooster/test_util.cpp +++ b/test/hotspot/gtest/jbooster/test_util.cpp @@ -35,10 +35,11 @@ #include "jbooster/utilities/scalarHashMap.inline.hpp" #include "runtime/os.inline.hpp" #include "runtime/thread.hpp" -#include "unittest.hpp" #include "utilities/exceptions.hpp" #include "utilities/stringUtils.hpp" +#include "unittest.hpp" + class ATestClass {}; template diff --git a/test/jdk/tools/jbooster/JBoosterConcurrentTest.java b/test/jdk/tools/jbooster/JBoosterConcurrentTest.java new file mode 100644 index 000000000..56423ba82 --- /dev/null +++ b/test/jdk/tools/jbooster/JBoosterConcurrentTest.java @@ -0,0 +1,106 @@ +/* + * Copyright (c) 2020, 2023, Huawei Technologies Co., Ltd. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +import java.net.ServerSocket; +import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import java.util.ArrayList; +import java.util.List; + +import jdk.test.lib.process.OutputAnalyzer; + +import static jdk.test.lib.Asserts.*; + +/* + * jbooster testing. + * @test + * @summary Test jbooster server + * @library ../lib + * /test/lib + * @modules jdk.jbooster + * @build SimpleClient + * @run main/othervm/timeout=5000 JBoosterConcurrentTest + */ +public class JBoosterConcurrentTest extends JBoosterTestBase { + public static final List SERVER_FAST_CLEANUP_ARGS = Stream.concat(SERVER_STANDARD_ARGS.stream(), Stream.of( + "--unused-cleanup-timeout=1" + )).collect(Collectors.toList()); + + private static void testProgramDataCleanup(TestContext ctx) throws Exception { + Process server = jbooster(ctx, List.of(), SERVER_FAST_CLEANUP_ARGS); + OutputAnalyzer out = new OutputAnalyzer(server); + server.waitFor(WAIT_START_TIME, TimeUnit.SECONDS); + + List apps = new ArrayList<>(); + final int rounds = 100; + final int concurrent = 5; + for (int r = 0; r < rounds; ++r) { + if (r % 10 == 0) { + System.out.println("Try " + (r + 1) + "round:"); + } + + ArrayList clientArgs = new ArrayList<>(CLIENT_STANDARD_ARGS); + clientArgs.add(clientArgs.size() - 1, "-XX:JBoosterProgramName=simple-" + r); + + for (int j = 0; j < concurrent; ++j) { + Process p = java(ctx, clientArgs); + apps.add(p); + } + + if (r % 10 == 0) { + for (Process p : apps) { + p.waitFor(WAIT_EXIT_TIME, TimeUnit.SECONDS); + assertEquals(p.exitValue(), 0, "good"); + } + apps.clear(); + } + } + + System.out.println("Get data 1:"); + writeToStdin(server, "d\n"); + + for (Process p : apps) { + p.waitFor(WAIT_EXIT_TIME, TimeUnit.SECONDS); + assertEquals(p.exitValue(), 0, "good"); + } + apps.clear(); + + System.out.println("Get data 2:"); + writeToStdin(server, "d\n"); + + Thread.sleep(5 * 60 * 1000); + + System.out.println("Get data 3:"); + writeToStdin(server, "d\n"); + writeToStdin(server, "d\n"); + writeToStdin(server, "d\n"); + exitNormallyByCommandQ(server); + out.shouldContain("JClientProgramData:\n -"); + out.shouldContain("JClientSessionData:\n\nJClientProgramData:\n\n".repeat(4)); + } + + public static void main(String[] args) throws Exception { + testCases(JBoosterConcurrentTest.class); + } +} diff --git a/test/jdk/tools/jbooster/TEST.properties b/test/jdk/tools/jbooster/TEST.properties new file mode 100644 index 000000000..e11b8706b --- /dev/null +++ b/test/jdk/tools/jbooster/TEST.properties @@ -0,0 +1 @@ +maxOutputSize = 2500000 -- 2.22.0