Skip to content

GitLab

T
tangled
Commit 8fad5216 authored by Marc Treib's avatar Marc Treib Committed by Commit Bot
Browse files
Options

Remove ModelAssociationManagerDelegate::OnSingleDataTypeAssociationDone 

It was always called directly from ModelAssociationManager::Associate().
The only non-trivial thing Associate() did was make sure it gets called
only once per datatype and ModelAssociationManager::Initialize().
DataTypeManagerImpl now takes care of that itself, by storing a map of
ConfigurationStats instead of a vector.
One small behavior difference is that the ordering of those
ConfigurationStats is not the same anymore. AFAICT, nothing depends on
the ordering, and anyway each ConfigurationStats entry contains
ordering information in the form of "types_configured_before", so this
should not matter.

Bug: 1102837
Change-Id: I95d71e8fa98b8b382bc182dc11e1fcbd6e58c4f8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2490109Reviewed-by: default avatarJan Krcal <jkrcal@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820192}
parent 58aed10b
Hide whitespace changes
Inline Side-by-side
Showing with 28 additions and 44 deletions
components/sync/driver/data_type_manager_impl.cc
View file @ 8fad5216
......@@ -682,6 +682,12 @@ void DataTypeManagerImpl::StartNextAssociation(AssociationGroup group) {
DVLOG(1) << "Associating " << ModelTypeSetToString(types_to_associate);
model_association_manager_.Associate(types_to_associate);
for (ModelType type : types_to_associate) {
if (ProtocolTypes().Has(type)) {
RecordConfigurationStats(type);
}
}
DCHECK(state_ == STOPPING || state_ == CONFIGURING);
if (state_ == STOPPING)
......@@ -736,33 +742,35 @@ void DataTypeManagerImpl::OnSingleDataTypeWillStop(ModelType type,
}
}
void DataTypeManagerImpl::OnSingleDataTypeAssociationDone(ModelType type) {
void DataTypeManagerImpl::RecordConfigurationStats(ModelType type) {
DCHECK(!association_types_queue_.empty());
if (!debug_info_listener_.IsInitialized())
return;
if (configuration_stats_.count(type) > 0)
return;
AssociationTypesInfo& info = association_types_queue_.front();
configuration_stats_.push_back(DataTypeConfigurationStats());
configuration_stats_.back().model_type = type;
configuration_stats_[type].model_type = type;
if (info.types.Has(type)) {
// Times in |info| only apply to non-slow types.
configuration_stats_.back().download_wait_time =
configuration_stats_[type].download_wait_time =
info.download_start_time - last_restart_time_;
if (info.first_sync_types.Has(type)) {
configuration_stats_.back().download_time =
configuration_stats_[type].download_time =
info.download_ready_time - info.download_start_time;
}
if (info.ready_types.Has(type)) {
configuration_stats_.back().association_wait_time_for_high_priority =
configuration_stats_[type].association_wait_time_for_high_priority =
info.ready_association_request_time - info.download_start_time;
} else {
configuration_stats_.back().association_wait_time_for_high_priority =
configuration_stats_[type].association_wait_time_for_high_priority =
info.full_association_request_time - info.download_ready_time;
}
configuration_stats_.back().high_priority_types_configured_before =
configuration_stats_[type].high_priority_types_configured_before =
info.high_priority_types_before;
configuration_stats_.back().same_priority_types_configured_before =
configuration_stats_[type].same_priority_types_configured_before =
info.configured_types;
info.configured_types.Put(type);
}
......@@ -833,9 +841,15 @@ void DataTypeManagerImpl::NotifyDone(const ConfigureResult& raw_result) {
base::UmaHistogramLongTimes(prefix_uma + ".OK", configure_time);
if (debug_info_listener_.IsInitialized() &&
!configuration_stats_.empty()) {
std::vector<DataTypeConfigurationStats> stats;
for (auto& type_and_stat : configuration_stats_) {
// Note: |configuration_stats_| gets cleared below, so it's okay to
// destroy its contents here.
stats.push_back(std::move(type_and_stat.second));
}
debug_info_listener_.Call(
FROM_HERE, &DataTypeDebugInfoListener::OnDataTypeConfigureComplete,
configuration_stats_);
stats);
}
configuration_stats_.clear();
break;
......
components/sync/driver/data_type_manager_impl.h
View file @ 8fad5216
......@@ -57,7 +57,6 @@ class DataTypeManagerImpl : public DataTypeManager,
// |ModelAssociationManagerDelegate| implementation.
void OnAllDataTypesReadyForConfigure() override;
void OnSingleDataTypeAssociationDone(ModelType type) override;
void OnSingleDataTypeWillStop(ModelType type,
const SyncError& error) override;
......@@ -105,6 +104,8 @@ class DataTypeManagerImpl : public DataTypeManager,
UNREADY_AT_CONFIG,
};
void RecordConfigurationStats(ModelType type);
// Return model types in |state_map| that match |state|.
static ModelTypeSet GetDataTypesInState(
DataTypeConfigState state,
......@@ -264,7 +265,7 @@ class DataTypeManagerImpl : public DataTypeManager,
const DataTypeEncryptionHandler* encryption_handler_;
// Association and time stats of data type configuration.
std::vector<DataTypeConfigurationStats> configuration_stats_;
std::map<ModelType, DataTypeConfigurationStats> configuration_stats_;
// Configuration process is started when ModelAssociationManager notifies
// DataTypeManager that all types are ready for configure.
......
components/sync/driver/model_association_manager.cc
View file @ 8fad5216
......@@ -195,26 +195,13 @@ void ModelAssociationManager::Associate(
DCHECK_EQ(INITIALIZED, state_);
DCHECK(notified_about_ready_for_configure_);
DVLOG(1) << "Starting association for "
<< ModelTypeSetToString(types_to_associate);
ModelTypeSet associating_types = types_to_associate;
associating_types.RetainAll(desired_types_);
associating_types.RemoveAll(associated_types_);
DCHECK(loaded_types_.HasAll(associating_types));
// Associate types in specified order.
for (ModelType type : kStartOrder) {
if (associating_types.Has(type)) {
associated_types_.Put(type);
associating_types.Remove(type);
if (ProtocolTypes().Has(type))
delegate_->OnSingleDataTypeAssociationDone(type);
}
}
DCHECK(associating_types.Empty());
associated_types_.PutAll(associating_types);
}
void ModelAssociationManager::Stop(ShutdownReason shutdown_reason) {
......
components/sync/driver/model_association_manager.h
View file @ 8fad5216
......@@ -26,10 +26,6 @@ class ModelAssociationManagerDelegate {
// ModelAssociationManager::Initialize().
virtual void OnAllDataTypesReadyForConfigure() = 0;
// Called when model association (MergeDataAndStartSyncing) has completed
// for |type|, regardless of success or failure.
virtual void OnSingleDataTypeAssociationDone(ModelType type) = 0;
// Called when the ModelAssociationManager has decided it must stop |type|,
// likely because it is no longer a desired data type, sync is shutting down,
// or some error occurred during loading.
......
components/sync/driver/model_association_manager_unittest.cc
View file @ 8fad5216
......@@ -35,10 +35,6 @@ class MockModelAssociationManagerDelegate
MockModelAssociationManagerDelegate() = default;
~MockModelAssociationManagerDelegate() override = default;
MOCK_METHOD(void, OnAllDataTypesReadyForConfigure, (), (override));
MOCK_METHOD(void,
OnSingleDataTypeAssociationDone,
(ModelType type),
(override));
MOCK_METHOD(void,
OnSingleDataTypeWillStop,
(ModelType, const SyncError& error),
......@@ -354,8 +350,6 @@ TEST_F(SyncModelAssociationManagerTest, KeepsMetadataForPreferredDataType) {
ModelTypeSet desired_types = preferred_types;
EXPECT_CALL(delegate_, OnAllDataTypesReadyForConfigure());
EXPECT_CALL(delegate_, OnSingleDataTypeAssociationDone(BOOKMARKS));
EXPECT_CALL(delegate_, OnSingleDataTypeAssociationDone(APPS));
model_association_manager.Initialize(desired_types, preferred_types,
BuildConfigureContext());
......@@ -392,8 +386,6 @@ TEST_F(SyncModelAssociationManagerTest, ClearsMetadataForNotPreferredDataType) {
ModelTypeSet desired_types = preferred_types;
EXPECT_CALL(delegate_, OnAllDataTypesReadyForConfigure());
EXPECT_CALL(delegate_, OnSingleDataTypeAssociationDone(BOOKMARKS));
EXPECT_CALL(delegate_, OnSingleDataTypeAssociationDone(APPS));
model_association_manager.Initialize(desired_types, preferred_types,
BuildConfigureContext());
......@@ -437,8 +429,6 @@ TEST_F(SyncModelAssociationManagerTest,
configure_context.cache_guid = "test_cache_guid";
EXPECT_CALL(delegate_, OnAllDataTypesReadyForConfigure());
EXPECT_CALL(delegate_, OnSingleDataTypeAssociationDone(BOOKMARKS));
EXPECT_CALL(delegate_, OnSingleDataTypeAssociationDone(APPS));
model_association_manager.Initialize(desired_types, preferred_types,
configure_context);
......@@ -458,7 +448,6 @@ TEST_F(SyncModelAssociationManagerTest,
EXPECT_CALL(delegate_, OnSingleDataTypeWillStop(APPS, _));
EXPECT_CALL(delegate_, OnSingleDataTypeWillStop(BOOKMARKS, _));
EXPECT_CALL(delegate_, OnAllDataTypesReadyForConfigure());
EXPECT_CALL(delegate_, OnSingleDataTypeAssociationDone(BOOKMARKS));
model_association_manager.Initialize(desired_types, preferred_types,
configure_context);
......@@ -489,8 +478,6 @@ TEST_F(SyncModelAssociationManagerTest,
configure_context.cache_guid = "test_cache_guid";
EXPECT_CALL(delegate_, OnAllDataTypesReadyForConfigure());
EXPECT_CALL(delegate_, OnSingleDataTypeAssociationDone(BOOKMARKS));
EXPECT_CALL(delegate_, OnSingleDataTypeAssociationDone(APPS));
model_association_manager.Initialize(desired_types, preferred_types,
configure_context);
......@@ -510,7 +497,6 @@ TEST_F(SyncModelAssociationManagerTest,
EXPECT_CALL(delegate_, OnSingleDataTypeWillStop(APPS, _));
EXPECT_CALL(delegate_, OnSingleDataTypeWillStop(BOOKMARKS, _));
EXPECT_CALL(delegate_, OnAllDataTypesReadyForConfigure());
EXPECT_CALL(delegate_, OnSingleDataTypeAssociationDone(BOOKMARKS));
model_association_manager.Initialize(desired_types, preferred_types,
configure_context);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment