From 96e76ef3611e3befcb1e2aa9d3abf3988b6a5613 Mon Sep 17 00:00:00 2001 From: Taku Kudo Date: Mon, 13 Nov 2023 02:21:37 +0000 Subject: [PATCH] Removed EngineBuilderInterface. We do not need to have an interface class if they are only used for dependency injections and unittesting. Interface is used to provide polymorphic behaviors. PiperOrigin-RevId: 581810906 --- src/engine/BUILD.bazel | 12 --- src/engine/engine.gyp | 2 + src/engine/engine_builder.h | 37 ++++++-- src/engine/engine_builder_interface.h | 89 ------------------ src/session/BUILD.bazel | 6 +- src/session/session_handler.cc | 14 ++- src/session/session_handler.h | 11 +-- src/session/session_handler_test.cc | 129 +++++++++++++------------- 8 files changed, 113 insertions(+), 187 deletions(-) delete mode 100644 src/engine/engine_builder_interface.h diff --git a/src/engine/BUILD.bazel b/src/engine/BUILD.bazel index e6f0901e0..332b877c7 100644 --- a/src/engine/BUILD.bazel +++ b/src/engine/BUILD.bazel @@ -40,24 +40,12 @@ package( ], ) -mozc_cc_library( - name = "engine_builder_interface", - hdrs = ["engine_builder_interface.h"], - deps = [ - ":engine_interface", - "//base:thread", - "//protocol:engine_builder_cc_proto", - "@com_google_absl//absl/strings", - ], -) - mozc_cc_library( name = "engine_builder", srcs = ["engine_builder.cc"], hdrs = ["engine_builder.h"], deps = [ ":engine", - ":engine_builder_interface", ":engine_interface", "//base:file_util", "//base:hash", diff --git a/src/engine/engine.gyp b/src/engine/engine.gyp index 8a65c4941..a8b9ac6e7 100644 --- a/src/engine/engine.gyp +++ b/src/engine/engine.gyp @@ -92,6 +92,7 @@ '../data_manager/oss/oss_data_manager.gyp:oss_data_manager', '../prediction/prediction.gyp:prediction', 'engine', + 'engine_builder' ], }, { @@ -106,6 +107,7 @@ '../dictionary/dictionary_base.gyp:pos_matcher', '../request/request.gyp:conversion_request', 'engine', + 'engine_builder' ], }, { diff --git a/src/engine/engine_builder.h b/src/engine/engine_builder.h index 231857bf3..5982ec91d 100644 --- a/src/engine/engine_builder.h +++ b/src/engine/engine_builder.h @@ -36,7 +36,6 @@ #include "base/thread.h" #include "data_manager/data_manager.h" -#include "engine/engine_builder_interface.h" #include "engine/engine_interface.h" #include "protocol/engine_builder.pb.h" #include "absl/base/thread_annotations.h" @@ -44,18 +43,44 @@ namespace mozc { -class EngineBuilder : public EngineBuilderInterface { +class EngineBuilder { public: EngineBuilder() = default; EngineBuilder(const EngineBuilder &) = delete; EngineBuilder &operator=(const EngineBuilder &) = delete; - ~EngineBuilder() override = default; + virtual ~EngineBuilder() = default; - uint64_t RegisterRequest(const EngineReloadRequest &request) override; + struct EngineResponse { + uint64_t id = 0; // engine id. Fingerprint of EngineReloadRequest. + EngineReloadResponse response; + std::unique_ptr engine; + }; + + // Wrapped with BackgroundFuture so the data loading is + // executed asynchronously. + using EngineResponseFuture = BackgroundFuture; + + // Accepts engine reload request and immediately returns the engine id with + // the highest priority defined as follows: + // - Request with higher request priority (e.g., downloaded > bundled) + // - When the priority is the same, the request registered last. + // The engine id 0 is reserved for unused engine. + virtual uint64_t RegisterRequest(const EngineReloadRequest &request); - uint64_t UnregisterRequest(uint64_t id) override; + // Unregister the request associated with the `id` and immediately returns + // the new engine id after the unregistration. This method is usually called + // to notify the request is not processed due to model loading failures and + // avoid the multiple loading operations. Client needs to load or use the + // engine of returned id. The unregistered request will not be accepted after + // calling this method. + virtual uint64_t UnregisterRequest(uint64_t id); - std::unique_ptr Build(uint64_t id) const override; + // Builds the new engine associated with `id`. + // This method returns the future object immediately. + // Since BackgroundFuture is not movable/copyable, we wrap it with + // std::unique_ptr. This method doesn't return nullptr. All errors + // are stored in EngineReloadResponse::response::status. + virtual std::unique_ptr Build(uint64_t id) const; void Clear(); diff --git a/src/engine/engine_builder_interface.h b/src/engine/engine_builder_interface.h deleted file mode 100644 index 55a928fbf..000000000 --- a/src/engine/engine_builder_interface.h +++ /dev/null @@ -1,89 +0,0 @@ -// Copyright 2010-2021, Google Inc. -// All rights reserved. -// -// Redistribution and use in source and binary forms, with or without -// modification, are permitted provided that the following conditions are -// met: -// -// * Redistributions of source code must retain the above copyright -// notice, this list of conditions and the following disclaimer. -// * Redistributions in binary form must reproduce the above -// copyright notice, this list of conditions and the following disclaimer -// in the documentation and/or other materials provided with the -// distribution. -// * Neither the name of Google Inc. nor the names of its -// contributors may be used to endorse or promote products derived from -// this software without specific prior written permission. -// -// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS -// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT -// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR -// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT -// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, -// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT -// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, -// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY -// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT -// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE -// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - -#ifndef MOZC_ENGINE_ENGINE_BUILDER_INTERFACE_H_ -#define MOZC_ENGINE_ENGINE_BUILDER_INTERFACE_H_ - -#include -#include - -#include "base/thread.h" -#include "engine/engine_interface.h" -#include "protocol/engine_builder.pb.h" -#include "absl/strings/string_view.h" - -namespace mozc { - -// Defines interface to build Engine instance in asynchronous way. -class EngineBuilderInterface { - public: - EngineBuilderInterface(const EngineBuilderInterface &) = delete; - EngineBuilderInterface &operator=(const EngineBuilderInterface &) = delete; - - virtual ~EngineBuilderInterface() = default; - - struct EngineResponse { - uint64_t id = 0; // engine id. Fingerprint of EngineReloadRequest. - EngineReloadResponse response; - std::unique_ptr engine; - }; - - // Wrapped with BackgroundFuture so the data loading is - // executed asynchronously. - using EngineResponseFuture = BackgroundFuture; - - // Accepts engine reload request and immediately returns the engine id with - // the highest priority defined as follows: - // - Request with higher request priority (e.g., downloaded > bundled) - // - When the priority is the same, the request registered last. - // The engine id 0 is reserved for unused engine. - virtual uint64_t RegisterRequest(const EngineReloadRequest &request) = 0; - - // Unregister the request associated with the `id` and immediately returns - // the new engine id after the unregistration. This method is usually called - // to notify the request is not processed due to model loading failures and - // avoid the multiple loading operations. Client needs to load or use the - // engine of returned id. The unregistered request will not be accepted after - // calling this method. - virtual uint64_t UnregisterRequest(uint64_t id) = 0; - - // Builds the new engine associated with `id`. - // This method returns the future object immediately. - // Since BackgroundFuture is not movable/copyable, we wrap it with - // std::unique_ptr. This method doesn't return nullptr. All errors - // are stored in EngineReloadResponse::response::status. - virtual std::unique_ptr Build(uint64_t id) const = 0; - - protected: - EngineBuilderInterface() = default; -}; - -} // namespace mozc - -#endif // MOZC_ENGINE_ENGINE_BUILDER_INTERFACE_H_ diff --git a/src/session/BUILD.bazel b/src/session/BUILD.bazel index 2b974c6b1..def05a013 100644 --- a/src/session/BUILD.bazel +++ b/src/session/BUILD.bazel @@ -322,7 +322,7 @@ mozc_cc_library( "//config:config_handler", "//converter:converter_interface", "//dictionary:user_dictionary_session_handler", - "//engine:engine_builder_interface", + "//engine:engine_builder", "//engine:engine_interface", "//engine:user_data_manager_interface", "//protocol:commands_cc_proto", @@ -377,7 +377,7 @@ mozc_cc_library( "//config:config_handler", "//converter:converter_interface", "//dictionary:user_dictionary_session_handler", - "//engine:engine_builder_interface", + "//engine:engine_builder", "//engine:engine_interface", "//prediction:user_history_predictor", "//protocol:commands_cc_proto", @@ -415,7 +415,7 @@ mozc_cc_test( "//config:config_handler", "//converter:segments", "//engine", - "//engine:engine_builder_interface", + "//engine:engine_builder", "//engine:engine_interface", "//engine:engine_mock", "//engine:engine_stub", diff --git a/src/session/session_handler.cc b/src/session/session_handler.cc index 3720eb9a9..6bf5288b9 100644 --- a/src/session/session_handler.cc +++ b/src/session/session_handler.cc @@ -48,7 +48,7 @@ #include "config/character_form_manager.h" #include "config/config_handler.h" #include "dictionary/user_dictionary_session_handler.h" -#include "engine/engine_builder_interface.h" +#include "engine/engine_builder.h" #include "engine/engine_interface.h" #include "engine/user_data_manager_interface.h" #include "protocol/commands.pb.h" @@ -133,18 +133,16 @@ bool IsApplicationAlive(const session::SessionInterface *session) { } // namespace SessionHandler::SessionHandler(std::unique_ptr engine) { - Init(std::move(engine), std::unique_ptr()); + Init(std::move(engine), std::unique_ptr()); } -SessionHandler::SessionHandler( - std::unique_ptr engine, - std::unique_ptr engine_builder) { +SessionHandler::SessionHandler(std::unique_ptr engine, + std::unique_ptr engine_builder) { Init(std::move(engine), std::move(engine_builder)); } -void SessionHandler::Init( - std::unique_ptr engine, - std::unique_ptr engine_builder) { +void SessionHandler::Init(std::unique_ptr engine, + std::unique_ptr engine_builder) { is_available_ = false; max_session_size_ = 0; last_session_empty_time_ = Clock::GetAbslTime(); diff --git a/src/session/session_handler.h b/src/session/session_handler.h index 1fe3507f4..a27535fdb 100644 --- a/src/session/session_handler.h +++ b/src/session/session_handler.h @@ -39,7 +39,7 @@ #include "composer/table.h" #include "dictionary/user_dictionary_session_handler.h" -#include "engine/engine_builder_interface.h" +#include "engine/engine_builder.h" #include "engine/engine_interface.h" #include "protocol/commands.pb.h" #include "protocol/config.pb.h" @@ -65,7 +65,7 @@ class SessionHandler : public SessionHandlerInterface { public: explicit SessionHandler(std::unique_ptr engine); SessionHandler(std::unique_ptr engine, - std::unique_ptr engine_builder); + std::unique_ptr engine_builder); SessionHandler(const SessionHandler &) = delete; SessionHandler &operator=(const SessionHandler &) = delete; ~SessionHandler() override; @@ -99,7 +99,7 @@ class SessionHandler : public SessionHandlerInterface { using SessionElement = SessionMap::Element; void Init(std::unique_ptr engine, - std::unique_ptr engine_builder); + std::unique_ptr engine_builder); // Updates the config, if the |command| contains the config. void MaybeUpdateConfig(commands::Command *command); @@ -154,7 +154,7 @@ class SessionHandler : public SessionHandlerInterface { absl::Time last_create_session_time_ = absl::InfinitePast(); std::unique_ptr engine_; - std::unique_ptr engine_builder_; + std::unique_ptr engine_builder_; std::unique_ptr observer_handler_; std::unique_ptr user_dictionary_session_handler_; @@ -162,8 +162,7 @@ class SessionHandler : public SessionHandlerInterface { std::unique_ptr request_; std::unique_ptr config_; std::unique_ptr key_map_manager_; - std::unique_ptr - engine_response_future_; + std::unique_ptr engine_response_future_; // used only in unittest to perform blocking behavior. bool always_wait_for_engine_response_future_ = false; diff --git a/src/session/session_handler_test.cc b/src/session/session_handler_test.cc index ee69bf309..4c4fcaee7 100644 --- a/src/session/session_handler_test.cc +++ b/src/session/session_handler_test.cc @@ -44,7 +44,7 @@ #include "config/config_handler.h" #include "converter/segments.h" #include "engine/engine.h" -#include "engine/engine_builder_interface.h" +#include "engine/engine_builder.h" #include "engine/engine_interface.h" #include "engine/engine_mock.h" #include "engine/engine_stub.h" @@ -79,7 +79,7 @@ using ::testing::_; using ::testing::InSequence; using ::testing::Return; -class MockEngineBuilder : public EngineBuilderInterface { +class MockEngineBuilder : public EngineBuilder { public: MOCK_METHOD(uint64_t, RegisterRequest, (const EngineReloadRequest &), (override)); @@ -572,7 +572,7 @@ TEST_F(SessionHandlerTest, SyncDataTest) { handler.EvalCommand(&command); } -// Tests the interaction with EngineBuilderInterface for successful Engine +// Tests the interaction with EngineBuilder for successful Engine // reload event. TEST_F(SessionHandlerTest, EngineReloadSuccessfulScenarioTest) { MockEngineBuilder *engine_builder = new MockEngineBuilder(); @@ -583,16 +583,16 @@ TEST_F(SessionHandlerTest, EngineReloadSuccessfulScenarioTest) { EXPECT_CALL(*engine_builder, RegisterRequest(_)).WillRepeatedly(Return(1)); EXPECT_CALL(*engine_builder, Build(1)) .WillOnce(Return( - std::make_unique< - BackgroundFuture>([&]() { - // takes 0.1 seconds to make engine. - absl::SleepFor(absl::Milliseconds(100)); - EngineBuilderInterface::EngineResponse result; - result.id = 1; - result.response.set_status(EngineReloadResponse::RELOAD_READY); - result.engine = std::move(new_engine); - return result; - }))); + std::make_unique>( + [&]() { + // takes 0.1 seconds to make engine. + absl::SleepFor(absl::Milliseconds(100)); + EngineBuilder::EngineResponse result; + result.id = 1; + result.response.set_status(EngineReloadResponse::RELOAD_READY); + result.engine = std::move(new_engine); + return result; + }))); SessionHandler handler(std::make_unique(), std::unique_ptr(engine_builder)); @@ -630,15 +630,15 @@ TEST_F(SessionHandlerTest, EngineUpdateSuccessfulScenarioTest) { EXPECT_CALL(*engine_builder, Build(1)) .WillOnce(Return( - std::make_unique< - BackgroundFuture>([&]() { - absl::SleepFor(absl::Milliseconds(100)); - EngineBuilderInterface::EngineResponse result; - result.id = 1; - result.response.set_status(EngineReloadResponse::RELOAD_READY); - result.engine = std::move(new_engine1); - return result; - }))); + std::make_unique>( + [&]() { + absl::SleepFor(absl::Milliseconds(100)); + EngineBuilder::EngineResponse result; + result.id = 1; + result.response.set_status(EngineReloadResponse::RELOAD_READY); + result.engine = std::move(new_engine1); + return result; + }))); SessionHandler handler(std::make_unique(), std::unique_ptr(engine_builder)); @@ -656,15 +656,15 @@ TEST_F(SessionHandlerTest, EngineUpdateSuccessfulScenarioTest) { EXPECT_CALL(*engine_builder, Build(2)) .WillOnce(Return( - std::make_unique< - BackgroundFuture>([&]() { - absl::SleepFor(absl::Milliseconds(100)); - EngineBuilderInterface::EngineResponse result; - result.id = 2; - result.response.set_status(EngineReloadResponse::RELOAD_READY); - result.engine = std::move(new_engine2); - return result; - }))); + std::make_unique>( + [&]() { + absl::SleepFor(absl::Milliseconds(100)); + EngineBuilder::EngineResponse result; + result.id = 2; + result.response.set_status(EngineReloadResponse::RELOAD_READY); + result.engine = std::move(new_engine2); + return result; + }))); // engine_id = 2 ASSERT_EQ(SendDummyEngineCommand(&handler), EngineReloadResponse::ACCEPTED); @@ -674,7 +674,7 @@ TEST_F(SessionHandlerTest, EngineUpdateSuccessfulScenarioTest) { EXPECT_EQ(new_engine_ptr2, &handler.engine()); } -// Tests the interaction with EngineBuilderInterface in the situation where +// Tests the interaction with EngineBuilder in the situation where // requested data is broken. TEST_F(SessionHandlerTest, EngineReloadInvalidDataTest) { MockEngineBuilder *engine_builder = new MockEngineBuilder(); @@ -692,14 +692,14 @@ TEST_F(SessionHandlerTest, EngineReloadInvalidDataTest) { EXPECT_CALL(*engine_builder, Build(1)) .WillOnce(Return( - std::make_unique< - BackgroundFuture>([&]() { - absl::SleepFor(absl::Milliseconds(100)); - EngineBuilderInterface::EngineResponse result; - result.id = 1; - result.response.set_status(EngineReloadResponse::DATA_BROKEN); - return result; - }))); + std::make_unique>( + [&]() { + absl::SleepFor(absl::Milliseconds(100)); + EngineBuilder::EngineResponse result; + result.id = 1; + result.response.set_status(EngineReloadResponse::DATA_BROKEN); + return result; + }))); EXPECT_CALL(*engine_builder, UnregisterRequest(1)).WillOnce(Return(0)); // Build() is called, but it returns invalid engine, so new engine is not @@ -740,19 +740,22 @@ TEST_F(SessionHandlerTest, EngineRollbackDataTest) { for (int eid = 3; eid >= 1; --eid) { // Rollback as 3 -> 2 -> 1. 1 is only valid engine. EXPECT_CALL(*engine_builder, Build(eid)) - .WillOnce(Return(std::make_unique>([&]() { - absl::SleepFor(absl::Milliseconds(100)); - EngineBuilderInterface::EngineResponse result; - result.id = eid; - if (eid == 1) { - result.response.set_status(EngineReloadResponse::RELOAD_READY); - result.engine = std::move(new_engine); - } else { - result.response.set_status(EngineReloadResponse::DATA_BROKEN); - } - return result; - }))); + .WillOnce(Return( + std::make_unique>( + [&]() { + absl::SleepFor(absl::Milliseconds(100)); + EngineBuilder::EngineResponse result; + result.id = eid; + if (eid == 1) { + result.response.set_status( + EngineReloadResponse::RELOAD_READY); + result.engine = std::move(new_engine); + } else { + result.response.set_status( + EngineReloadResponse::DATA_BROKEN); + } + return result; + }))); // Engine of 3, and 2 are unregistered. // The second best id (2, and 1) are used. if (eid > 1) { @@ -768,7 +771,7 @@ TEST_F(SessionHandlerTest, EngineRollbackDataTest) { EXPECT_EQ(new_engine_ptr, &handler.engine()); } -// Tests the interaction with EngineBuilderInterface in the situation where +// Tests the interaction with EngineBuilder in the situation where // sessions exist in create session event. TEST_F(SessionHandlerTest, EngineReloadSessionExistsTest) { auto old_engine = std::make_unique(); @@ -781,15 +784,15 @@ TEST_F(SessionHandlerTest, EngineReloadSessionExistsTest) { EXPECT_CALL(*engine_builder, RegisterRequest(_)).WillOnce(Return(1)); EXPECT_CALL(*engine_builder, Build(1)) .WillOnce(Return( - std::make_unique< - BackgroundFuture>([&]() { - absl::SleepFor(absl::Milliseconds(100)); - EngineBuilderInterface::EngineResponse result; - result.id = 1; - result.response.set_status(EngineReloadResponse::RELOAD_READY); - result.engine = std::move(new_engine); - return result; - }))); + std::make_unique>( + [&]() { + absl::SleepFor(absl::Milliseconds(100)); + EngineBuilder::EngineResponse result; + result.id = 1; + result.response.set_status(EngineReloadResponse::RELOAD_READY); + result.engine = std::move(new_engine); + return result; + }))); SessionHandler handler(std::move(old_engine), std::unique_ptr(engine_builder));