Commit 1a3039bf authored by Findit's avatar Findit

Revert "[Dolphin] Add a sequence checker"

This reverts commit bef7a422.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 683057 as the
culprit for failures in the build cycles as shown on:
https://analysis.chromium.org/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2JlZjdhNDIyZTk4N2U1NTcxYWYxZjZhNWM0MGEzMjQxMzFlNjk3ZDAM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.memory/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/34678

Sample Failed Step: unit_tests

Original change's description:
> [Dolphin] Add a sequence checker
> 
> The RecurrenceRanker isn't threadsafe, so we should formalize this.
> This CL adds a sequence DCHECK to all public methods (and a couple
> of private callbacks, for clarity).
> 
> Bug: 921444
> Change-Id: Ib52f8e33be82de147efd23e67c424083ff980184
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1729991
> Reviewed-by: Charles . <charleszhao@chromium.org>
> Commit-Queue: Tony Yeoman <tby@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#683057}


Change-Id: Idfac688d72ba6a356259a575209b755ec340b7ac
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 921444
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1730620
Cr-Commit-Position: refs/heads/master@{#683131}
parent 782b983e
...@@ -166,7 +166,6 @@ RecurrenceRanker::RecurrenceRanker(const std::string& model_identifier, ...@@ -166,7 +166,6 @@ RecurrenceRanker::RecurrenceRanker(const std::string& model_identifier,
TimeDelta::FromSeconds(config.min_seconds_between_saves())), TimeDelta::FromSeconds(config.min_seconds_between_saves())),
time_of_last_save_(Time::Now()), time_of_last_save_(Time::Now()),
weak_factory_(this) { weak_factory_(this) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
task_runner_ = base::CreateSequencedTaskRunnerWithTraits( task_runner_ = base::CreateSequencedTaskRunnerWithTraits(
{base::TaskPriority::BEST_EFFORT, base::MayBlock(), {base::TaskPriority::BEST_EFFORT, base::MayBlock(),
base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN}); base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN});
...@@ -202,7 +201,6 @@ RecurrenceRanker::~RecurrenceRanker() = default; ...@@ -202,7 +201,6 @@ RecurrenceRanker::~RecurrenceRanker() = default;
void RecurrenceRanker::OnLoadProtoFromDiskComplete( void RecurrenceRanker::OnLoadProtoFromDiskComplete(
std::unique_ptr<RecurrenceRankerProto> proto) { std::unique_ptr<RecurrenceRankerProto> proto) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
load_from_disk_completed_ = true; load_from_disk_completed_ = true;
LogInitializationStatus(model_identifier_, LogInitializationStatus(model_identifier_,
InitializationStatus::kInitialized); InitializationStatus::kInitialized);
...@@ -244,7 +242,6 @@ void RecurrenceRanker::OnLoadProtoFromDiskComplete( ...@@ -244,7 +242,6 @@ void RecurrenceRanker::OnLoadProtoFromDiskComplete(
void RecurrenceRanker::Record(const std::string& target, void RecurrenceRanker::Record(const std::string& target,
const std::string& condition) { const std::string& condition) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!load_from_disk_completed_) if (!load_from_disk_completed_)
return; return;
LogUsage(model_identifier_, Usage::kRecord); LogUsage(model_identifier_, Usage::kRecord);
...@@ -255,7 +252,6 @@ void RecurrenceRanker::Record(const std::string& target, ...@@ -255,7 +252,6 @@ void RecurrenceRanker::Record(const std::string& target,
void RecurrenceRanker::RenameTarget(const std::string& target, void RecurrenceRanker::RenameTarget(const std::string& target,
const std::string& new_target) { const std::string& new_target) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!load_from_disk_completed_) if (!load_from_disk_completed_)
return; return;
LogUsage(model_identifier_, Usage::kRenameTarget); LogUsage(model_identifier_, Usage::kRenameTarget);
...@@ -265,7 +261,6 @@ void RecurrenceRanker::RenameTarget(const std::string& target, ...@@ -265,7 +261,6 @@ void RecurrenceRanker::RenameTarget(const std::string& target,
} }
void RecurrenceRanker::RemoveTarget(const std::string& target) { void RecurrenceRanker::RemoveTarget(const std::string& target) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
// TODO(tby): Find a solution to the edge case of a removal before disk // TODO(tby): Find a solution to the edge case of a removal before disk
// loading is complete, resulting in the remove getting dropped. // loading is complete, resulting in the remove getting dropped.
if (!load_from_disk_completed_) if (!load_from_disk_completed_)
...@@ -278,7 +273,6 @@ void RecurrenceRanker::RemoveTarget(const std::string& target) { ...@@ -278,7 +273,6 @@ void RecurrenceRanker::RemoveTarget(const std::string& target) {
void RecurrenceRanker::RenameCondition(const std::string& condition, void RecurrenceRanker::RenameCondition(const std::string& condition,
const std::string& new_condition) { const std::string& new_condition) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!load_from_disk_completed_) if (!load_from_disk_completed_)
return; return;
LogUsage(model_identifier_, Usage::kRenameCondition); LogUsage(model_identifier_, Usage::kRenameCondition);
...@@ -288,7 +282,6 @@ void RecurrenceRanker::RenameCondition(const std::string& condition, ...@@ -288,7 +282,6 @@ void RecurrenceRanker::RenameCondition(const std::string& condition,
} }
void RecurrenceRanker::RemoveCondition(const std::string& condition) { void RecurrenceRanker::RemoveCondition(const std::string& condition) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!load_from_disk_completed_) if (!load_from_disk_completed_)
return; return;
LogUsage(model_identifier_, Usage::kRemoveCondition); LogUsage(model_identifier_, Usage::kRemoveCondition);
...@@ -299,7 +292,6 @@ void RecurrenceRanker::RemoveCondition(const std::string& condition) { ...@@ -299,7 +292,6 @@ void RecurrenceRanker::RemoveCondition(const std::string& condition) {
std::map<std::string, float> RecurrenceRanker::Rank( std::map<std::string, float> RecurrenceRanker::Rank(
const std::string& condition) { const std::string& condition) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!load_from_disk_completed_) if (!load_from_disk_completed_)
return {}; return {};
LogUsage(model_identifier_, Usage::kRank); LogUsage(model_identifier_, Usage::kRank);
...@@ -321,7 +313,6 @@ std::map<std::string, float> RecurrenceRanker::Rank( ...@@ -321,7 +313,6 @@ std::map<std::string, float> RecurrenceRanker::Rank(
void RecurrenceRanker::MaybeCleanup(float proportion_valid, void RecurrenceRanker::MaybeCleanup(float proportion_valid,
const FrecencyStore::ScoreTable& targets) { const FrecencyStore::ScoreTable& targets) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (proportion_valid > kMinValidTargetProportionBeforeCleanup) if (proportion_valid > kMinValidTargetProportionBeforeCleanup)
return; return;
...@@ -334,7 +325,6 @@ void RecurrenceRanker::MaybeCleanup(float proportion_valid, ...@@ -334,7 +325,6 @@ void RecurrenceRanker::MaybeCleanup(float proportion_valid,
std::vector<std::pair<std::string, float>> RecurrenceRanker::RankTopN( std::vector<std::pair<std::string, float>> RecurrenceRanker::RankTopN(
int n, int n,
const std::string& condition) { const std::string& condition) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!load_from_disk_completed_) if (!load_from_disk_completed_)
return {}; return {};
...@@ -343,18 +333,15 @@ std::vector<std::pair<std::string, float>> RecurrenceRanker::RankTopN( ...@@ -343,18 +333,15 @@ std::vector<std::pair<std::string, float>> RecurrenceRanker::RankTopN(
std::map<std::string, FrecencyStore::ValueData>* std::map<std::string, FrecencyStore::ValueData>*
RecurrenceRanker::GetTargetData() { RecurrenceRanker::GetTargetData() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
return targets_->get_mutable_values(); return targets_->get_mutable_values();
} }
std::map<std::string, FrecencyStore::ValueData>* std::map<std::string, FrecencyStore::ValueData>*
RecurrenceRanker::GetConditionData() { RecurrenceRanker::GetConditionData() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
return conditions_->get_mutable_values(); return conditions_->get_mutable_values();
} }
void RecurrenceRanker::SaveToDisk() { void RecurrenceRanker::SaveToDisk() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (is_ephemeral_user_) if (is_ephemeral_user_)
return; return;
...@@ -367,14 +354,12 @@ void RecurrenceRanker::SaveToDisk() { ...@@ -367,14 +354,12 @@ void RecurrenceRanker::SaveToDisk() {
} }
void RecurrenceRanker::MaybeSave() { void RecurrenceRanker::MaybeSave() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (Time::Now() - time_of_last_save_ > min_seconds_between_saves_) { if (Time::Now() - time_of_last_save_ > min_seconds_between_saves_) {
SaveToDisk(); SaveToDisk();
} }
} }
void RecurrenceRanker::ToProto(RecurrenceRankerProto* proto) { void RecurrenceRanker::ToProto(RecurrenceRankerProto* proto) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
proto->set_config_hash(config_hash_); proto->set_config_hash(config_hash_);
predictor_->ToProto(proto->mutable_predictor()); predictor_->ToProto(proto->mutable_predictor());
targets_->ToProto(proto->mutable_targets()); targets_->ToProto(proto->mutable_targets());
......
...@@ -142,8 +142,6 @@ class RecurrenceRanker { ...@@ -142,8 +142,6 @@ class RecurrenceRanker {
const base::TimeDelta min_seconds_between_saves_; const base::TimeDelta min_seconds_between_saves_;
base::Time time_of_last_save_; base::Time time_of_last_save_;
SEQUENCE_CHECKER(sequence_checker_);
scoped_refptr<base::SequencedTaskRunner> task_runner_; scoped_refptr<base::SequencedTaskRunner> task_runner_;
base::WeakPtrFactory<RecurrenceRanker> weak_factory_; base::WeakPtrFactory<RecurrenceRanker> weak_factory_;
......
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