Skip to content

Commit

Permalink
Stop using raw pointer to share composer::Table.
Browse files Browse the repository at this point in the history
Background:
composer::Table is shared across different components.
Though we would like to avoid shared object, Table is not copyable so there is no other way to share them at this moment. The internal data of the Table is managed by std::unique_ptr, which makes simple copying impossible. Furthermore, copying the table every time would result in a significant performance degradation. Style guide says that we prefer to use std::shared_ptr for shared memory object.
PiperOrigin-RevId: 730072127
  • Loading branch information
taku910 authored and hiroyuki-komatsu committed Feb 23, 2025
1 parent 73d25e3 commit 5f789eb
Show file tree
Hide file tree
Showing 33 changed files with 557 additions and 529 deletions.
1 change: 1 addition & 0 deletions src/composer/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ mozc_cc_library(
"//composer/internal:special_key",
"//protocol:commands_cc_proto",
"//protocol:config_cc_proto",
"@com_google_absl//absl/base:no_destructor",
"@com_google_absl//absl/container:flat_hash_map",
"@com_google_absl//absl/container:flat_hash_set",
"@com_google_absl//absl/hash",
Expand Down
23 changes: 14 additions & 9 deletions src/composer/composer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@

#include <cstddef>
#include <cstdint>
#include <memory>
#include <string>
#include <tuple>
#include <unordered_map>
Expand Down Expand Up @@ -569,14 +570,16 @@ void ComposerData::GetSubTransliterations(
// Composer

Composer::Composer()
: Composer(Table::GetDefaultTable(), commands::Request::default_instance(),
: Composer(Table::GetSharedDefaultTable(),
commands::Request::default_instance(),
config::ConfigHandler::DefaultConfig()) {}

Composer::Composer(const commands::Request &request,
const config::Config &config)
: Composer(Table::GetDefaultTable(), request, config) {}
: Composer(Table::GetSharedDefaultTable(), request, config) {}

Composer::Composer(const Table &table, const commands::Request &request,
Composer::Composer(std::shared_ptr<const Table> table,
const commands::Request &request,
const config::Config &config)
: position_(0),
input_mode_(transliteration::HIRAGANA),
Expand All @@ -588,16 +591,17 @@ Composer::Composer(const Table &table, const commands::Request &request,
max_length_(kMaxPreeditLength),
request_(&request),
config_(&config),
table_(&table),
table_(table),
is_new_input_(true) {
SetInputMode(transliteration::HIRAGANA);
DCHECK(table_);
Reset();
}

// static
ComposerData Composer::CreateEmptyComposerData() {
static const absl::NoDestructor<Table> table;
static const absl::NoDestructor<Composition> composition(*table);
static const absl::NoDestructor<Composition> composition(
Table::GetSharedDefaultTable());
return ComposerData(*composition, 0, transliteration::HIRAGANA,
commands::Context::NORMAL, "", {});
}
Expand All @@ -624,9 +628,10 @@ void Composer::ReloadConfig() {

bool Composer::Empty() const { return (GetLength() == 0); }

void Composer::SetTable(const Table &table) {
table_ = &table;
composition_.SetTable(*table_);
void Composer::SetTable(std::shared_ptr<const Table> table) {
table_ = table;
DCHECK(table_);
composition_.SetTable(table_);
}

void Composer::SetRequest(const commands::Request &request) {
Expand Down
25 changes: 17 additions & 8 deletions src/composer/composer.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,12 @@ namespace mozc {
namespace composer {

// ComposerData is a data structure that represents the current state of the
// composer. It is used by Converter, Predictor and Rewriters as a const object.
// composer. It is used by Converter, Predictor and Rewriters as a const
// object.
class ComposerData {
public:
// This constructor is temporary and should be removed, when ConverterRequest
// is updated to use a const ComposerData.
// This constructor is temporary and should be removed, when
// ConverterRequest is updated to use a const ComposerData.
ABSL_DEPRECATED("Do not use this constructor except in converter_request.h")
ComposerData() = default;

Expand Down Expand Up @@ -144,8 +145,8 @@ class ComposerData {
};

// Composer is a class that manages the composing text. It provides methods to
// edit the text by users. Composer creates ComposerData as the snapshot of the
// current state of the composer.
// edit the text by users. Composer creates ComposerData as the snapshot of
// the current state of the composer.
class Composer final {
public:
// Pseudo commands in composer.
Expand All @@ -155,7 +156,7 @@ class Composer final {
};

Composer();
Composer(const Table &table, const commands::Request &request,
Composer(std::shared_ptr<const Table> table, const commands::Request &request,
const config::Config &config);
// This constructor is for testing.
ABSL_DEPRECATED("Use the constructor with Table")
Expand Down Expand Up @@ -188,7 +189,7 @@ class Composer final {
// Check the preedit string is empty or not.
bool Empty() const;

void SetTable(const Table &table);
void SetTable(std::shared_ptr<const Table> table);

void SetRequest(const commands::Request &request);
void SetConfig(const config::Config &config);
Expand Down Expand Up @@ -390,9 +391,17 @@ class Composer final {

size_t max_length_;

// TODO(all): Stop using raw pointer to achieve shared ownership.
const commands::Request *request_ = nullptr;
const config::Config *config_ = nullptr;
const Table *table_ = nullptr;

// Though we would like to avoid shared object, Table is not copyable so
// there is no other way to share them at this moment. The internal data
// of the Table is managed by std::unique_ptr, which makes simple copying
// impossible. Furthermore, copying the table every time would result in
// a significant performance degradation. Style guide says that we prefer
// to use std::shared_ptr for shared object.
std::shared_ptr<const Table> table_;

// Timestamp of last modified.
int64_t timestamp_msec_ = 0;
Expand Down
4 changes: 2 additions & 2 deletions src/composer/composer_main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ using ::mozc::config::Config;
int main(int argc, char **argv) {
mozc::InitMozc(argv[0], &argc, &argv);

mozc::composer::Table table;
table.LoadFromFile(absl::GetFlag(FLAGS_table).c_str());
auto table = std::make_shared<mozc::composer::Table>();
table->LoadFromFile(absl::GetFlag(FLAGS_table).c_str());
auto composer = std::make_unique<mozc::composer::Composer>(
table, Request::default_instance(), Config::default_instance());

Expand Down
12 changes: 6 additions & 6 deletions src/composer/composer_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,10 @@ class ComposerTest : public ::testing::Test {
~ComposerTest() override = default;

void SetUp() override {
table_ = std::make_unique<Table>();
table_ = std::make_shared<Table>();
config_ = std::make_unique<Config>();
request_ = std::make_unique<Request>();
composer_ = std::make_unique<Composer>(*table_, *request_, *config_);
composer_ = std::make_unique<Composer>(table_, *request_, *config_);
CharacterFormManager::GetCharacterFormManager()->SetDefaultRule();
}

Expand All @@ -123,7 +123,7 @@ class ComposerTest : public ::testing::Test {
}

std::unique_ptr<Composer> composer_;
std::unique_ptr<Table> table_;
std::shared_ptr<Table> table_;
std::unique_ptr<Request> request_;
std::unique_ptr<Config> config_;
};
Expand Down Expand Up @@ -854,7 +854,7 @@ TEST_F(ComposerTest, InsertCharacterKeyEventWithInputMode) {
EXPECT_EQ(composer_->GetInputMode(), transliteration::HIRAGANA);
}

composer_ = std::make_unique<Composer>(*table_, *request_, *config_);
composer_ = std::make_unique<Composer>(table_, *request_, *config_);

{
// "a" → "あ" (Hiragana)
Expand Down Expand Up @@ -1256,7 +1256,7 @@ TEST_F(ComposerTest, AutoIMETurnOffEnabled) {
EXPECT_EQ(composer_->GetInputMode(), transliteration::HIRAGANA);
}

composer_ = std::make_unique<Composer>(*table_, *request_, *config_);
composer_ = std::make_unique<Composer>(table_, *request_, *config_);

{ // google
InsertKey("g", composer_.get());
Expand Down Expand Up @@ -1321,7 +1321,7 @@ TEST_F(ComposerTest, AutoIMETurnOffEnabled) {
}

config_->set_shift_key_mode_switch(Config::OFF);
composer_ = std::make_unique<Composer>(*table_, *request_, *config_);
composer_ = std::make_unique<Composer>(table_, *request_, *config_);

{ // Google
InsertKey("G", composer_.get());
Expand Down
14 changes: 8 additions & 6 deletions src/composer/internal/char_chunk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "composer/internal/char_chunk.h"

#include <cstddef>
#include <memory>
#include <string>
#include <tuple>
#include <utility>
Expand Down Expand Up @@ -111,13 +112,14 @@ bool GetFromPending(const Table *table, const absl::string_view key,
} // namespace

CharChunk::CharChunk(Transliterators::Transliterator transliterator,
const Table &table)
: table_(&table), transliterator_(transliterator) {
std::shared_ptr<const Table> table)
: table_(table), transliterator_(transliterator) {
DCHECK(table_);
DCHECK_NE(transliterator, Transliterators::LOCAL);
}

CharChunk::CharChunk(Transliterators::Transliterator transliterator)
: transliterator_(transliterator) {
: table_(Table::GetSharedDefaultTable()), transliterator_(transliterator) {
DCHECK_NE(transliterator, Transliterators::LOCAL);
}

Expand Down Expand Up @@ -244,7 +246,7 @@ absl::btree_set<std::string> CharChunk::GetExpandedResults() const {
continue;
}
absl::btree_set<std::string> loop_result;
if (!GetFromPending(table_, entry->pending(), kMaxRecursion,
if (!GetFromPending(table_.get(), entry->pending(), kMaxRecursion,
&loop_result)) {
continue;
}
Expand All @@ -261,7 +263,7 @@ bool CharChunk::IsAppendable(Transliterators::Transliterator t12r,
const Table &table) const {
return !pending_.empty() &&
(t12r == Transliterators::LOCAL || t12r == transliterator_) &&
&table == table_;
&table == table_.get();
}

bool CharChunk::IsConvertible(Transliterators::Transliterator t12r,
Expand Down Expand Up @@ -565,7 +567,7 @@ absl::StatusOr<CharChunk> CharChunk::SplitChunk(
DeleteSpecialKeys(conversion_ + pending_), &raw_lhs, &raw_rhs,
&converted_lhs, &converted_rhs);

CharChunk left_new_chunk(transliterator_, *table_);
CharChunk left_new_chunk(transliterator_, table_);
left_new_chunk.set_raw(raw_lhs);
set_raw(std::move(raw_rhs));

Expand Down
14 changes: 8 additions & 6 deletions src/composer/internal/char_chunk.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@
#include <tuple>
#include <utility>

#include "absl/container/btree_set.h"
#include "absl/base/attributes.h"
#include "absl/container/btree_set.h"
#include "absl/status/statusor.h"
#include "absl/strings/str_format.h"
#include "absl/strings/string_view.h"
Expand All @@ -56,7 +56,8 @@ namespace composer {
class CharChunk final {
public:
// LOCAL transliterator is not accepted.
CharChunk(Transliterators::Transliterator transliterator, const Table &table);
CharChunk(Transliterators::Transliterator transliterator,
std::shared_ptr<const Table> table);
// This constructor is for testing.
ABSL_DEPRECATED("Use the constructor with Table")
explicit CharChunk(Transliterators::Transliterator transliterator);
Expand Down Expand Up @@ -142,7 +143,7 @@ class CharChunk final {
Transliterators::Transliterator transliterator() const {
return transliterator_;
}
const Table *table() const { return table_; }
std::shared_ptr<const Table> table_for_testing() const { return table_; }

const std::string &raw() const { return raw_; }
template <typename String>
Expand Down Expand Up @@ -191,8 +192,9 @@ class CharChunk final {
absl::Format(&sink,
"table = %p, raw = %s, conversion = %s, pending = %s, "
"ambiguous = %s, transliterator = %v, attributes = %v",
chunk.table_, chunk.raw_, chunk.conversion_, chunk.pending_,
chunk.ambiguous_, chunk.transliterator_, chunk.attributes_);
chunk.table_.get(), chunk.raw_, chunk.conversion_,
chunk.pending_, chunk.ambiguous_, chunk.transliterator_,
chunk.attributes_);
}

// bool = should loop
Expand All @@ -202,7 +204,7 @@ class CharChunk final {
private:
void AddInputAndConvertedChar(CompositionInput *composition_input);

const Table *table_ = nullptr;
std::shared_ptr<const Table> table_;

// There are four variables to represent a composing text:
// `raw_`, `conversion_`, `pending_`, and `ambiguous_`.
Expand Down
Loading

0 comments on commit 5f789eb

Please sign in to comment.