diff options
author | Terry Wang <tytytyww@google.com> | 2023-05-15 17:09:06 -0700 |
---|---|---|
committer | Terry Wang <tytytyww@google.com> | 2023-05-15 17:09:06 -0700 |
commit | ed8f00e32ce543f464893527a9eb08c4bb40b664 (patch) | |
tree | cdbc350bd4fbbaf0233eaacbfb8034cc6f079fcd | |
parent | fb6eb3c7c025b798b13ae36b923aac8c6ebe24bd (diff) | |
download | icing-ed8f00e32ce543f464893527a9eb08c4bb40b664.tar.gz |
Update Icing from upstream.
Descriptions:
========================================================================
Minor fix for export to framework.
========================================================================
[Icing][version][3/x][ez] Call DataSync for version file
========================================================================
Add `use_read_only_search` flag for flag-guarding IcingSearchEngine::Search locking change in cl/506671441.
========================================================================
Bug: 146008613
Test: presubmit
Change-Id: Id863275cf5cc1ad7bcc69649527cc17981cc6cd2
-rw-r--r-- | icing/file/version-util.cc | 7 | ||||
-rw-r--r-- | icing/icing-search-engine.cc | 58 | ||||
-rw-r--r-- | icing/icing-search-engine.h | 24 | ||||
-rw-r--r-- | icing/icing-search-engine_search_test.cc | 99 | ||||
-rw-r--r-- | icing/schema/backup-schema-producer_test.cc | 13 | ||||
-rw-r--r-- | icing/schema/schema-store.cc | 1 | ||||
-rw-r--r-- | proto/icing/proto/search.proto | 10 | ||||
-rw-r--r-- | synced_AOSP_CL_number.txt | 2 |
8 files changed, 196 insertions, 18 deletions
diff --git a/icing/file/version-util.cc b/icing/file/version-util.cc index 468bde5..f477072 100644 --- a/icing/file/version-util.cc +++ b/icing/file/version-util.cc @@ -69,8 +69,11 @@ libtextclassifier3::StatusOr<VersionInfo> ReadVersion( libtextclassifier3::Status WriteVersion(const Filesystem& filesystem, const std::string& version_file_path, const VersionInfo& version_info) { - if (!filesystem.PWrite(version_file_path.c_str(), /*offset=*/0, &version_info, - sizeof(VersionInfo))) { + ScopedFd scoped_fd(filesystem.OpenForWrite(version_file_path.c_str())); + if (!scoped_fd.is_valid() || + !filesystem.PWrite(scoped_fd.get(), /*offset=*/0, &version_info, + sizeof(VersionInfo)) || + !filesystem.DataSync(scoped_fd.get())) { return absl_ports::InternalError("Fail to write version"); } return libtextclassifier3::Status::OK; diff --git a/icing/icing-search-engine.cc b/icing/icing-search-engine.cc index e7b6ae9..2dc58a2 100644 --- a/icing/icing-search-engine.cc +++ b/icing/icing-search-engine.cc @@ -1807,19 +1807,61 @@ libtextclassifier3::Status IcingSearchEngine::InternalPersistToDisk( SearchResultProto IcingSearchEngine::Search( const SearchSpecProto& search_spec, const ScoringSpecProto& scoring_spec, const ResultSpecProto& result_spec) { + if (search_spec.use_read_only_search()) { + return SearchLockedShared(search_spec, scoring_spec, result_spec); + } else { + return SearchLockedExclusive(search_spec, scoring_spec, result_spec); + } +} + +SearchResultProto IcingSearchEngine::SearchLockedShared( + const SearchSpecProto& search_spec, const ScoringSpecProto& scoring_spec, + const ResultSpecProto& result_spec) { + std::unique_ptr<Timer> overall_timer = clock_->GetNewTimer(); + + // Only acquire an overall read-lock for this implementation. Finer-grained + // locks are implemented around code paths that write changes to Icing's data + // members. + absl_ports::shared_lock l(&mutex_); + int64_t lock_acquisition_latency = overall_timer->GetElapsedMilliseconds(); + + SearchResultProto result_proto = + InternalSearch(search_spec, scoring_spec, result_spec); + + result_proto.mutable_query_stats()->set_lock_acquisition_latency_ms( + lock_acquisition_latency); + result_proto.mutable_query_stats()->set_latency_ms( + overall_timer->GetElapsedMilliseconds()); + return result_proto; +} + +SearchResultProto IcingSearchEngine::SearchLockedExclusive( + const SearchSpecProto& search_spec, const ScoringSpecProto& scoring_spec, + const ResultSpecProto& result_spec) { + std::unique_ptr<Timer> overall_timer = clock_->GetNewTimer(); + + // Acquire the overall write-lock for this locked implementation. + absl_ports::unique_lock l(&mutex_); + int64_t lock_acquisition_latency = overall_timer->GetElapsedMilliseconds(); + + SearchResultProto result_proto = + InternalSearch(search_spec, scoring_spec, result_spec); + + result_proto.mutable_query_stats()->set_lock_acquisition_latency_ms( + lock_acquisition_latency); + result_proto.mutable_query_stats()->set_latency_ms( + overall_timer->GetElapsedMilliseconds()); + return result_proto; +} + +SearchResultProto IcingSearchEngine::InternalSearch( + const SearchSpecProto& search_spec, const ScoringSpecProto& scoring_spec, + const ResultSpecProto& result_spec) { SearchResultProto result_proto; StatusProto* result_status = result_proto.mutable_status(); QueryStatsProto* query_stats = result_proto.mutable_query_stats(); query_stats->set_query_length(search_spec.query().length()); - ScopedTimer overall_timer(clock_->GetNewTimer(), [query_stats](int64_t t) { - query_stats->set_latency_ms(t); - }); - // Only an overall read-lock is required here. A finer-grained write-lock is - // provided around the LiteIndex. - absl_ports::shared_lock l(&mutex_); - query_stats->set_lock_acquisition_latency_ms( - overall_timer.timer().GetElapsedMilliseconds()); if (!initialized_) { result_status->set_code(StatusProto::FAILED_PRECONDITION); result_status->set_message("IcingSearchEngine has not been initialized!"); diff --git a/icing/icing-search-engine.h b/icing/icing-search-engine.h index 4192169..1a5d130 100644 --- a/icing/icing-search-engine.h +++ b/icing/icing-search-engine.h @@ -567,6 +567,30 @@ class IcingSearchEngine { InitializeStatsProto* initialize_stats) ICING_EXCLUSIVE_LOCKS_REQUIRED(mutex_); + // Implementation of IcingSearchEngine::Search that only grabs the overall + // read-lock, allowing for parallel non-exclusive operations. + // This implementation is used if search_spec.use_read_only_search is true. + SearchResultProto SearchLockedShared(const SearchSpecProto& search_spec, + const ScoringSpecProto& scoring_spec, + const ResultSpecProto& result_spec) + ICING_LOCKS_EXCLUDED(mutex_); + + // Implementation of IcingSearchEngine::Search that requires the overall + // write lock. No other operations of any kind can be executed in parallel if + // this version is used. + // This implementation is used if search_spec.use_read_only_search is false. + SearchResultProto SearchLockedExclusive(const SearchSpecProto& search_spec, + const ScoringSpecProto& scoring_spec, + const ResultSpecProto& result_spec) + ICING_LOCKS_EXCLUDED(mutex_); + + // Helper method for the actual work to Search. We need this separate + // method to manage locking for Search. + SearchResultProto InternalSearch(const SearchSpecProto& search_spec, + const ScoringSpecProto& scoring_spec, + const ResultSpecProto& result_spec) + ICING_SHARED_LOCKS_REQUIRED(mutex_); + // Processes query and scores according to the specs. It is a helper function // (called by Search) to process and score normal query and the nested child // query for join search. diff --git a/icing/icing-search-engine_search_test.cc b/icing/icing-search-engine_search_test.cc index cada6c7..dc31ff0 100644 --- a/icing/icing-search-engine_search_test.cc +++ b/icing/icing-search-engine_search_test.cc @@ -409,6 +409,57 @@ TEST_P(IcingSearchEngineSearchTest, SearchReturnsOneResult) { expected_search_result_proto)); } +TEST_P(IcingSearchEngineSearchTest, SearchReturnsOneResult_readOnlyFalse) { + auto fake_clock = std::make_unique<FakeClock>(); + fake_clock->SetTimerElapsedMilliseconds(1000); + TestIcingSearchEngine icing(GetDefaultIcingOptions(), + std::make_unique<Filesystem>(), + std::make_unique<IcingFilesystem>(), + std::move(fake_clock), GetTestJniCache()); + ASSERT_THAT(icing.Initialize().status(), ProtoIsOk()); + ASSERT_THAT(icing.SetSchema(CreateMessageSchema()).status(), ProtoIsOk()); + + DocumentProto document_one = CreateMessageDocument("namespace", "uri1"); + ASSERT_THAT(icing.Put(document_one).status(), ProtoIsOk()); + + DocumentProto document_two = CreateMessageDocument("namespace", "uri2"); + ASSERT_THAT(icing.Put(document_two).status(), ProtoIsOk()); + + SearchSpecProto search_spec; + search_spec.set_term_match_type(TermMatchType::PREFIX); + search_spec.set_query("message"); + search_spec.set_search_type(GetParam()); + search_spec.set_use_read_only_search(false); + + ResultSpecProto result_spec; + result_spec.set_num_per_page(1); + + SearchResultProto expected_search_result_proto; + expected_search_result_proto.mutable_status()->set_code(StatusProto::OK); + *expected_search_result_proto.mutable_results()->Add()->mutable_document() = + document_two; + + SearchResultProto search_result_proto = + icing.Search(search_spec, GetDefaultScoringSpec(), result_spec); + EXPECT_THAT(search_result_proto.status(), ProtoIsOk()); + + EXPECT_THAT(search_result_proto.query_stats().latency_ms(), Eq(1000)); + EXPECT_THAT(search_result_proto.query_stats().parse_query_latency_ms(), + Eq(1000)); + EXPECT_THAT(search_result_proto.query_stats().scoring_latency_ms(), Eq(1000)); + EXPECT_THAT(search_result_proto.query_stats().ranking_latency_ms(), Eq(1000)); + EXPECT_THAT(search_result_proto.query_stats().document_retrieval_latency_ms(), + Eq(1000)); + EXPECT_THAT(search_result_proto.query_stats().lock_acquisition_latency_ms(), + Eq(1000)); + + // The token is a random number so we don't verify it. + expected_search_result_proto.set_next_page_token( + search_result_proto.next_page_token()); + EXPECT_THAT(search_result_proto, EqualsSearchResultIgnoreStatsAndScores( + expected_search_result_proto)); +} + TEST_P(IcingSearchEngineSearchTest, SearchZeroResultLimitReturnsEmptyResults) { IcingSearchEngine icing(GetDefaultIcingOptions(), GetTestJniCache()); ASSERT_THAT(icing.Initialize().status(), ProtoIsOk()); @@ -430,6 +481,28 @@ TEST_P(IcingSearchEngineSearchTest, SearchZeroResultLimitReturnsEmptyResults) { } TEST_P(IcingSearchEngineSearchTest, + SearchZeroResultLimitReturnsEmptyResults_readOnlyFalse) { + IcingSearchEngine icing(GetDefaultIcingOptions(), GetTestJniCache()); + ASSERT_THAT(icing.Initialize().status(), ProtoIsOk()); + + SearchSpecProto search_spec; + search_spec.set_term_match_type(TermMatchType::PREFIX); + search_spec.set_query(""); + search_spec.set_search_type(GetParam()); + search_spec.set_use_read_only_search(false); + + ResultSpecProto result_spec; + result_spec.set_num_per_page(0); + + SearchResultProto expected_search_result_proto; + expected_search_result_proto.mutable_status()->set_code(StatusProto::OK); + SearchResultProto actual_results = + icing.Search(search_spec, GetDefaultScoringSpec(), result_spec); + EXPECT_THAT(actual_results, EqualsSearchResultIgnoreStatsAndScores( + expected_search_result_proto)); +} + +TEST_P(IcingSearchEngineSearchTest, SearchNegativeResultLimitReturnsInvalidArgument) { IcingSearchEngine icing(GetDefaultIcingOptions(), GetTestJniCache()); ASSERT_THAT(icing.Initialize().status(), ProtoIsOk()); @@ -454,6 +527,32 @@ TEST_P(IcingSearchEngineSearchTest, } TEST_P(IcingSearchEngineSearchTest, + SearchNegativeResultLimitReturnsInvalidArgument_readOnlyFalse) { + IcingSearchEngine icing(GetDefaultIcingOptions(), GetTestJniCache()); + ASSERT_THAT(icing.Initialize().status(), ProtoIsOk()); + + SearchSpecProto search_spec; + search_spec.set_term_match_type(TermMatchType::PREFIX); + search_spec.set_query(""); + search_spec.set_search_type(GetParam()); + search_spec.set_use_read_only_search(false); + + ResultSpecProto result_spec; + result_spec.set_num_per_page(-5); + + SearchResultProto expected_search_result_proto; + expected_search_result_proto.mutable_status()->set_code( + StatusProto::INVALID_ARGUMENT); + expected_search_result_proto.mutable_status()->set_message( + "ResultSpecProto.num_per_page cannot be negative."); + SearchResultProto actual_results = + icing.Search(search_spec, GetDefaultScoringSpec(), result_spec); + EXPECT_THAT(actual_results, EqualsSearchResultIgnoreStatsAndScores( + expected_search_result_proto)); +} + + +TEST_P(IcingSearchEngineSearchTest, SearchNonPositivePageTotalBytesLimitReturnsInvalidArgument) { IcingSearchEngine icing(GetDefaultIcingOptions(), GetTestJniCache()); ASSERT_THAT(icing.Initialize().status(), ProtoIsOk()); diff --git a/icing/schema/backup-schema-producer_test.cc b/icing/schema/backup-schema-producer_test.cc index 424fec0..b0e793c 100644 --- a/icing/schema/backup-schema-producer_test.cc +++ b/icing/schema/backup-schema-producer_test.cc @@ -19,6 +19,7 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" #include "icing/file/filesystem.h" +#include "icing/portable/equals-proto.h" #include "icing/proto/schema.pb.h" #include "icing/schema-builder.h" #include "icing/schema/schema-type-manager.h" @@ -202,7 +203,7 @@ TEST_F(BackupSchemaProducerTest, RemoveRfc822) { .SetCardinality(CARDINALITY_OPTIONAL) .SetDataType(TYPE_STRING))) .Build(); - EXPECT_THAT(backup, testing::EqualsProto(expected_backup)); + EXPECT_THAT(backup, portable_equals_proto::EqualsProto(expected_backup)); } TEST_F(BackupSchemaProducerTest, MakeExtraStringIndexedPropertiesUnindexed) { @@ -284,7 +285,7 @@ TEST_F(BackupSchemaProducerTest, MakeExtraStringIndexedPropertiesUnindexed) { .AddProperty(unindexed_string_property_builder.SetName("prop19")) .Build(); SchemaProto expected_backup = SchemaBuilder().AddType(expected_type).Build(); - EXPECT_THAT(backup, testing::EqualsProto(expected_backup)); + EXPECT_THAT(backup, portable_equals_proto::EqualsProto(expected_backup)); } TEST_F(BackupSchemaProducerTest, MakeExtraIntIndexedPropertiesUnindexed) { @@ -366,7 +367,7 @@ TEST_F(BackupSchemaProducerTest, MakeExtraIntIndexedPropertiesUnindexed) { .AddProperty(unindexed_int_property_builder.SetName("prop19")) .Build(); SchemaProto expected_backup = SchemaBuilder().AddType(expected_type).Build(); - EXPECT_THAT(backup, testing::EqualsProto(expected_backup)); + EXPECT_THAT(backup, portable_equals_proto::EqualsProto(expected_backup)); } TEST_F(BackupSchemaProducerTest, MakeExtraDocumentIndexedPropertiesUnindexed) { @@ -438,7 +439,7 @@ TEST_F(BackupSchemaProducerTest, MakeExtraDocumentIndexedPropertiesUnindexed) { .Build(); SchemaProto expected_backup = SchemaBuilder().AddType(expected_typeA).AddType(typeB).Build(); - EXPECT_THAT(backup, testing::EqualsProto(expected_backup)); + EXPECT_THAT(backup, portable_equals_proto::EqualsProto(expected_backup)); } TEST_F(BackupSchemaProducerTest, MakeRfcPropertiesUnindexedFirst) { @@ -518,7 +519,7 @@ TEST_F(BackupSchemaProducerTest, MakeRfcPropertiesUnindexedFirst) { .AddProperty(indexed_string_property_builder.SetName("prop16")) .Build(); SchemaProto expected_backup = SchemaBuilder().AddType(expected_typeA).Build(); - EXPECT_THAT(backup, testing::EqualsProto(expected_backup)); + EXPECT_THAT(backup, portable_equals_proto::EqualsProto(expected_backup)); } TEST_F(BackupSchemaProducerTest, MakeExtraPropertiesUnindexedMultipleTypes) { @@ -621,7 +622,7 @@ TEST_F(BackupSchemaProducerTest, MakeExtraPropertiesUnindexedMultipleTypes) { .Build(); SchemaProto expected_backup = SchemaBuilder().AddType(expected_typeA).AddType(typeB).Build(); - EXPECT_THAT(backup, testing::EqualsProto(expected_backup)); + EXPECT_THAT(backup, portable_equals_proto::EqualsProto(expected_backup)); } } // namespace diff --git a/icing/schema/schema-store.cc b/icing/schema/schema-store.cc index f262206..bcc7c2c 100644 --- a/icing/schema/schema-store.cc +++ b/icing/schema/schema-store.cc @@ -15,6 +15,7 @@ #include "icing/schema/schema-store.h" #include <algorithm> +#include <cinttypes> #include <cstdint> #include <limits> #include <memory> diff --git a/proto/icing/proto/search.proto b/proto/icing/proto/search.proto index e5ad269..fca669a 100644 --- a/proto/icing/proto/search.proto +++ b/proto/icing/proto/search.proto @@ -27,7 +27,7 @@ option java_multiple_files = true; option objc_class_prefix = "ICNG"; // Client-supplied specifications on what documents to retrieve. -// Next tag: 9 +// Next tag: 10 message SearchSpecProto { // REQUIRED: The "raw" query string that users may type. For example, "cat" // will search for documents with the term cat in it. @@ -94,6 +94,14 @@ message SearchSpecProto { // Features enabled in this search spec. repeated string enabled_features = 8; + + // OPTIONAL: Whether to use the read-only implementation of + // IcingSearchEngine::Search. + // The read-only version enables multiple queries to be performed concurrently + // as it only acquires the read lock at IcingSearchEngine's level. + // Finer-grained locks are implemented around code paths that write changes to + // Icing during Search. + optional bool use_read_only_search = 9 [default = true]; } // Client-supplied specifications on what to include/how to format the search diff --git a/synced_AOSP_CL_number.txt b/synced_AOSP_CL_number.txt index ae59ff7..8807fa7 100644 --- a/synced_AOSP_CL_number.txt +++ b/synced_AOSP_CL_number.txt @@ -1 +1 @@ -set(synced_AOSP_CL_number=531296607) +set(synced_AOSP_CL_number=532167936) |