Commit 3b9b1f9a authored by Sebastien Marchand's avatar Sebastien Marchand Committed by Commit Bot

[PM] base for the footprint based memory pressure signals

This CL adds a new policy that will emit critical memory pressure
signals when Chrome's total PMF exceeds is too high. "Too high" is
defined as 1.5x the total amount of RAM on the machine by default (but
this value can be modified via Finch). This code is behind a feature
flag, in the disabled by default state.

Bug: 1055889
Change-Id: If57e7ac5dce601fc83ef50c50838617ece35dcb3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2044330Reviewed-by: default avatarErik Chen <erikchen@chromium.org>
Reviewed-by: default avatarFrançois Doray <fdoray@chromium.org>
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#744829}
parent 12f84491
...@@ -1144,6 +1144,8 @@ jumbo_static_library("browser") { ...@@ -1144,6 +1144,8 @@ jumbo_static_library("browser") {
"performance_manager/decorators/process_metrics_decorator.h", "performance_manager/decorators/process_metrics_decorator.h",
"performance_manager/decorators/process_priority_aggregator.cc", "performance_manager/decorators/process_priority_aggregator.cc",
"performance_manager/decorators/process_priority_aggregator.h", "performance_manager/decorators/process_priority_aggregator.h",
"performance_manager/graph/policies/high_pmf_memory_pressure_policy.cc",
"performance_manager/graph/policies/high_pmf_memory_pressure_policy.h",
"performance_manager/graph/policies/policy_features.cc", "performance_manager/graph/policies/policy_features.cc",
"performance_manager/graph/policies/policy_features.h", "performance_manager/graph/policies/policy_features.h",
"performance_manager/graph/policies/working_set_trimmer_policy.cc", "performance_manager/graph/policies/working_set_trimmer_policy.cc",
...@@ -1152,6 +1154,8 @@ jumbo_static_library("browser") { ...@@ -1152,6 +1154,8 @@ jumbo_static_library("browser") {
"performance_manager/graph/policies/working_set_trimmer_policy_chromeos.h", "performance_manager/graph/policies/working_set_trimmer_policy_chromeos.h",
"performance_manager/graph/policies/working_set_trimmer_policy_win.cc", "performance_manager/graph/policies/working_set_trimmer_policy_win.cc",
"performance_manager/graph/policies/working_set_trimmer_policy_win.h", "performance_manager/graph/policies/working_set_trimmer_policy_win.h",
"performance_manager/mechanisms/high_pmf_memory_pressure_signals.cc",
"performance_manager/mechanisms/high_pmf_memory_pressure_signals.h",
"performance_manager/mechanisms/working_set_trimmer.cc", "performance_manager/mechanisms/working_set_trimmer.cc",
"performance_manager/mechanisms/working_set_trimmer.h", "performance_manager/mechanisms/working_set_trimmer.h",
"performance_manager/mechanisms/working_set_trimmer_chromeos.cc", "performance_manager/mechanisms/working_set_trimmer_chromeos.cc",
......
...@@ -15,6 +15,7 @@ ...@@ -15,6 +15,7 @@
#include "chrome/browser/performance_manager/decorators/helpers/page_live_state_decorator_helper.h" #include "chrome/browser/performance_manager/decorators/helpers/page_live_state_decorator_helper.h"
#include "chrome/browser/performance_manager/decorators/page_aggregator.h" #include "chrome/browser/performance_manager/decorators/page_aggregator.h"
#include "chrome/browser/performance_manager/decorators/process_metrics_decorator.h" #include "chrome/browser/performance_manager/decorators/process_metrics_decorator.h"
#include "chrome/browser/performance_manager/graph/policies/high_pmf_memory_pressure_policy.h"
#include "chrome/browser/performance_manager/graph/policies/policy_features.h" #include "chrome/browser/performance_manager/graph/policies/policy_features.h"
#include "chrome/browser/performance_manager/graph/policies/urgent_page_discarding_policy.h" #include "chrome/browser/performance_manager/graph/policies/urgent_page_discarding_policy.h"
#include "chrome/browser/performance_manager/graph/policies/working_set_trimmer_policy.h" #include "chrome/browser/performance_manager/graph/policies/working_set_trimmer_policy.h"
...@@ -109,6 +110,13 @@ void ChromeBrowserMainExtraPartsPerformanceManager::CreatePoliciesAndDecorators( ...@@ -109,6 +110,13 @@ void ChromeBrowserMainExtraPartsPerformanceManager::CreatePoliciesAndDecorators(
graph->PassToGraph( graph->PassToGraph(
std::make_unique<performance_manager::metrics::MemoryPressureMetrics>()); std::make_unique<performance_manager::metrics::MemoryPressureMetrics>());
if (base::FeatureList::IsEnabled(
performance_manager::features::kHighPMFMemoryPressureSignals)) {
graph->PassToGraph(
std::make_unique<
performance_manager::policies::HighPMFMemoryPressurePolicy>());
}
} }
content::FeatureObserverClient* content::FeatureObserverClient*
......
...@@ -48,11 +48,13 @@ void ProcessMetricsDecorator::StartTimer() { ...@@ -48,11 +48,13 @@ void ProcessMetricsDecorator::StartTimer() {
base::TimeDelta refresh_period = kDefaultRefreshTimerPeriod; base::TimeDelta refresh_period = kDefaultRefreshTimerPeriod;
#if !defined(OS_ANDROID) #if !defined(OS_ANDROID)
// Bump the refresh frequency when urgent discarding is done from the graph as // Bump the refresh frequency when urgent discarding is done from the graph or
// this relies on relatively fresh data. // when emitting memory pressure signals on high PMF as these features relies
// on relatively fresh data.
// TODO(sebmarchand): Measure the performance impact of this. // TODO(sebmarchand): Measure the performance impact of this.
if (base::FeatureList::IsEnabled( if (base::FeatureList::IsEnabled(
features::kUrgentDiscardingFromPerformanceManager)) { features::kUrgentDiscardingFromPerformanceManager) ||
base::FeatureList::IsEnabled(features::kHighPMFMemoryPressureSignals)) {
refresh_period = kFastRefreshTimerPeriod; refresh_period = kFastRefreshTimerPeriod;
} }
#endif #endif
......
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "chrome/browser/performance_manager/graph/policies/high_pmf_memory_pressure_policy.h"
#include "base/bind.h"
#include "base/memory/memory_pressure_listener.h"
#include "base/metrics/field_trial_params.h"
#include "base/process/process_metrics.h"
#include "chrome/browser/performance_manager/graph/policies/policy_features.h"
#include "chrome/browser/performance_manager/mechanisms/high_pmf_memory_pressure_signals.h"
#include "components/performance_manager/public/graph/process_node.h"
namespace performance_manager {
namespace policies {
namespace {
// The factor that will be applied to the total amount of RAM to establish the
// PMF limit.
static constexpr base::FeatureParam<double> kRAMRatioPMFLimitFactor{
&performance_manager::features::kHighPMFMemoryPressureSignals,
"RAMRatioPMFLimitFactor", 1.5};
} // namespace
HighPMFMemoryPressurePolicy::HighPMFMemoryPressurePolicy() = default;
HighPMFMemoryPressurePolicy::~HighPMFMemoryPressurePolicy() = default;
void HighPMFMemoryPressurePolicy::OnPassedToGraph(Graph* graph) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
graph->AddSystemNodeObserver(this);
graph_ = graph;
base::SystemMemoryInfoKB mem_info = {};
if (base::GetSystemMemoryInfo(&mem_info))
pmf_limit_kb_ = mem_info.total * kRAMRatioPMFLimitFactor.Get();
mechanism_ = std::make_unique<mechanism::HighPMFMemoryPressureSignals>();
}
void HighPMFMemoryPressurePolicy::OnTakenFromGraph(Graph* graph) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
graph->RemoveSystemNodeObserver(this);
graph_ = nullptr;
pmf_limit_kb_ = kInvalidPMFLimitValue;
mechanism_.reset();
}
void HighPMFMemoryPressurePolicy::OnProcessMemoryMetricsAvailable(
const SystemNode* unused) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (pmf_limit_kb_ == kInvalidPMFLimitValue)
return;
int total_pmf_kb = 0;
MemoryPressureLevel pressure_level =
MemoryPressureLevel::MEMORY_PRESSURE_LEVEL_NONE;
auto process_nodes = graph_->GetAllProcessNodes();
for (const auto* node : process_nodes) {
total_pmf_kb += node->GetPrivateFootprintKb();
if (total_pmf_kb >= pmf_limit_kb_) {
pressure_level = MemoryPressureLevel::MEMORY_PRESSURE_LEVEL_CRITICAL;
break;
}
}
mechanism_->SetPressureLevel(pressure_level);
}
} // namespace policies
} // namespace performance_manager
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef CHROME_BROWSER_PERFORMANCE_MANAGER_GRAPH_POLICIES_HIGH_PMF_MEMORY_PRESSURE_POLICY_H_
#define CHROME_BROWSER_PERFORMANCE_MANAGER_GRAPH_POLICIES_HIGH_PMF_MEMORY_PRESSURE_POLICY_H_
#include "base/memory/memory_pressure_listener.h"
#include "base/sequence_checker.h"
#include "components/performance_manager/public/graph/graph.h"
#include "components/performance_manager/public/graph/system_node.h"
namespace performance_manager {
class Graph;
namespace mechanism {
class HighPMFMemoryPressureSignals;
}
namespace policies {
// The HighPMFMemoryPressurePolicy will emit critical memory pressure signal
// when Chrome's total PMF exceeds a given threshold.
class HighPMFMemoryPressurePolicy : public GraphOwned,
public SystemNode::ObserverDefaultImpl {
public:
HighPMFMemoryPressurePolicy();
~HighPMFMemoryPressurePolicy() override;
HighPMFMemoryPressurePolicy(const HighPMFMemoryPressurePolicy& other) =
delete;
HighPMFMemoryPressurePolicy& operator=(const HighPMFMemoryPressurePolicy&) =
delete;
// GraphOwned implementation:
void OnPassedToGraph(Graph* graph) override;
void OnTakenFromGraph(Graph* graph) override;
// SystemNode::ObserverDefaultImpl:
void OnProcessMemoryMetricsAvailable(const SystemNode* system_node) override;
void set_pmf_limit_for_testing(int pmf_limit_kb) {
pmf_limit_kb_ = pmf_limit_kb;
}
private:
using MemoryPressureLevel = base::MemoryPressureListener::MemoryPressureLevel;
const int kInvalidPMFLimitValue = 0;
int pmf_limit_kb_ = kInvalidPMFLimitValue;
std::unique_ptr<mechanism::HighPMFMemoryPressureSignals> mechanism_;
Graph* graph_ = nullptr;
SEQUENCE_CHECKER(sequence_checker_);
};
} // namespace policies
} // namespace performance_manager
#endif // CHROME_BROWSER_PERFORMANCE_MANAGER_GRAPH_POLICIES_HIGH_PMF_MEMORY_PRESSURE_POLICY_H_
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "chrome/browser/performance_manager/graph/policies/high_pmf_memory_pressure_policy.h"
#include "base/bind.h"
#include "base/memory/memory_pressure_listener.h"
#include "base/run_loop.h"
#include "base/test/bind_test_util.h"
#include "base/util/memory_pressure/multi_source_memory_pressure_monitor.h"
#include "components/performance_manager/graph/graph_impl.h"
#include "components/performance_manager/test_support/graph_test_harness.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace performance_manager {
namespace policies {
using HighPMFMemoryPressurePolicyTest = GraphTestHarness;
TEST_F(HighPMFMemoryPressurePolicyTest, EndToEnd) {
// Create the policy and pass it to the graph.
auto policy = std::make_unique<HighPMFMemoryPressurePolicy>();
auto* policy_raw = policy.get();
graph()->PassToGraph(std::move(policy));
const int kPMFLimitKb = 100 * 1024;
policy_raw->set_pmf_limit_for_testing(kPMFLimitKb);
// Create a MemoryPressureListener object that will record that a critical
// memory pressure signal has been emitted.
base::RunLoop run_loop;
auto quit_closure = run_loop.QuitClosure();
bool memory_pressure_listener_called = false;
util::MultiSourceMemoryPressureMonitor memory_pressure_monitor;
base::MemoryPressureListener memory_pressure_listener(
base::BindLambdaForTesting(
[&](base::MemoryPressureListener::MemoryPressureLevel level) {
EXPECT_EQ(base::MemoryPressureListener::MemoryPressureLevel::
MEMORY_PRESSURE_LEVEL_CRITICAL,
level);
memory_pressure_listener_called = true;
quit_closure.Run();
}));
auto process_node = CreateNode<performance_manager::ProcessNodeImpl>();
process_node->set_private_footprint_kb(kPMFLimitKb - 1);
graph()->FindOrCreateSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
// Make sure that no task get posted to the memory pressure monitor to record
// a memory pressure event.
task_env().RunUntilIdle();
EXPECT_FALSE(memory_pressure_listener_called);
EXPECT_EQ(base::MemoryPressureListener::MemoryPressureLevel::
MEMORY_PRESSURE_LEVEL_NONE,
memory_pressure_monitor.GetCurrentPressureLevel());
process_node->set_private_footprint_kb(kPMFLimitKb);
graph()->FindOrCreateSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
// Wait for the MemoryPressureListener to be called.
run_loop.Run();
EXPECT_TRUE(memory_pressure_listener_called);
EXPECT_EQ(base::MemoryPressureListener::MemoryPressureLevel::
MEMORY_PRESSURE_LEVEL_CRITICAL,
memory_pressure_monitor.GetCurrentPressureLevel());
memory_pressure_listener_called = false;
process_node->set_private_footprint_kb(kPMFLimitKb - 1);
graph()->FindOrCreateSystemNodeImpl()->OnProcessMemoryMetricsAvailable();
// Make sure that the pressure level goes back to NONE.
task_env().RunUntilIdle();
EXPECT_FALSE(memory_pressure_listener_called);
EXPECT_EQ(base::MemoryPressureListener::MemoryPressureLevel::
MEMORY_PRESSURE_LEVEL_NONE,
memory_pressure_monitor.GetCurrentPressureLevel());
graph()->TakeFromGraph(policy_raw);
// The policy's voter post a task to the UI thread on destruction, wait for
// this to complete.
task_env().RunUntilIdle();
}
} // namespace policies
} // namespace performance_manager
...@@ -89,5 +89,8 @@ const base::Feature kUrgentDiscardingFromPerformanceManager{ ...@@ -89,5 +89,8 @@ const base::Feature kUrgentDiscardingFromPerformanceManager{
base::FEATURE_DISABLED_BY_DEFAULT}; base::FEATURE_DISABLED_BY_DEFAULT};
#endif #endif
const base::Feature kHighPMFMemoryPressureSignals{
"HighPMFMemoryPressureSignals", base::FEATURE_DISABLED_BY_DEFAULT};
} // namespace features } // namespace features
} // namespace performance_manager } // namespace performance_manager
...@@ -86,6 +86,10 @@ extern const base::Feature kPageFreezingFromPerformanceManager; ...@@ -86,6 +86,10 @@ extern const base::Feature kPageFreezingFromPerformanceManager;
extern const base::Feature kUrgentDiscardingFromPerformanceManager; extern const base::Feature kUrgentDiscardingFromPerformanceManager;
#endif #endif
// Feature that controls whether or not memory pressure signals will be emitted
// when the total PMF is too high.
extern const base::Feature kHighPMFMemoryPressureSignals;
} // namespace features } // namespace features
} // namespace performance_manager } // namespace performance_manager
......
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "chrome/browser/performance_manager/mechanisms/high_pmf_memory_pressure_signals.h"
#include <memory>
#include "base/bind.h"
#include "base/memory/memory_pressure_monitor.h"
#include "base/task/post_task.h"
#include "base/util/memory_pressure/memory_pressure_voter.h"
#include "base/util/memory_pressure/multi_source_memory_pressure_monitor.h"
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
namespace performance_manager {
namespace mechanism {
// Wrapper around a MemoryPressureVoter living on the UI thread.
class MemoryPressureVoterOnUIThread {
public:
MemoryPressureVoterOnUIThread() = default;
virtual ~MemoryPressureVoterOnUIThread() {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
}
MemoryPressureVoterOnUIThread(const MemoryPressureVoterOnUIThread& other) =
delete;
MemoryPressureVoterOnUIThread& operator=(
const MemoryPressureVoterOnUIThread&) = delete;
void InitOnUIThread() {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
voter_ = static_cast<util::MultiSourceMemoryPressureMonitor*>(
base::MemoryPressureMonitor::Get())
->CreateVoter();
}
void SetVote(base::MemoryPressureListener::MemoryPressureLevel level,
bool notify) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
voter_->SetVote(level, notify);
}
private:
std::unique_ptr<util::MemoryPressureVoter> voter_;
};
HighPMFMemoryPressureSignals::HighPMFMemoryPressureSignals() {
ui_thread_voter_ = std::make_unique<MemoryPressureVoterOnUIThread>();
base::PostTask(FROM_HERE, {content::BrowserThread::UI},
base::BindOnce(&MemoryPressureVoterOnUIThread::InitOnUIThread,
base::Unretained(ui_thread_voter_.get())));
}
HighPMFMemoryPressureSignals::~HighPMFMemoryPressureSignals() {
base::DeleteSoon(FROM_HERE, {content::BrowserThread::UI},
std::move(ui_thread_voter_));
}
void HighPMFMemoryPressureSignals::SetPressureLevel(MemoryPressureLevel level) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
bool notify = level == MemoryPressureLevel::MEMORY_PRESSURE_LEVEL_CRITICAL;
base::PostTask(
FROM_HERE, {content::BrowserThread::UI},
base::BindOnce(&MemoryPressureVoterOnUIThread::SetVote,
base::Unretained(ui_thread_voter_.get()), level, notify));
pressure_level_ = level;
}
} // namespace mechanism
} // namespace performance_manager
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef CHROME_BROWSER_PERFORMANCE_MANAGER_MECHANISMS_HIGH_PMF_MEMORY_PRESSURE_SIGNALS_H_
#define CHROME_BROWSER_PERFORMANCE_MANAGER_MECHANISMS_HIGH_PMF_MEMORY_PRESSURE_SIGNALS_H_
#include "base/memory/memory_pressure_listener.h"
#include "base/sequence_checker.h"
namespace performance_manager {
namespace mechanism {
class MemoryPressureVoterOnUIThread;
// HighPMFMemoryPressureSignals is meant to be used by a
// HighPMFMemoryPressurePolicy to notify the global MemoryPressureMonitor when
// the total PMF of Chrome exceeds a threshold.
class HighPMFMemoryPressureSignals {
public:
using MemoryPressureLevel = base::MemoryPressureListener::MemoryPressureLevel;
HighPMFMemoryPressureSignals();
~HighPMFMemoryPressureSignals();
HighPMFMemoryPressureSignals(const HighPMFMemoryPressureSignals& other) =
delete;
HighPMFMemoryPressureSignals& operator=(const HighPMFMemoryPressureSignals&) =
delete;
// Should be called each time the memory pressure level computed by
// HighPMFMemoryPressurePolicy changes.
void SetPressureLevel(MemoryPressureLevel level);
private:
MemoryPressureLevel pressure_level_ =
MemoryPressureLevel::MEMORY_PRESSURE_LEVEL_NONE;
std::unique_ptr<MemoryPressureVoterOnUIThread> ui_thread_voter_;
SEQUENCE_CHECKER(sequence_checker_);
};
} // namespace mechanism
} // namespace performance_manager
#endif // CHROME_BROWSER_PERFORMANCE_MANAGER_MECHANISMS_HIGH_PMF_MEMORY_PRESSURE_SIGNALS_H_
...@@ -3309,6 +3309,7 @@ test("unit_tests") { ...@@ -3309,6 +3309,7 @@ test("unit_tests") {
"../browser/performance_manager/decorators/process_metrics_decorator_unittest.cc", "../browser/performance_manager/decorators/process_metrics_decorator_unittest.cc",
"../browser/performance_manager/decorators/process_priority_aggregator_unittest.cc", "../browser/performance_manager/decorators/process_priority_aggregator_unittest.cc",
"../browser/performance_manager/graph/policies/dynamic_tcmalloc_policy_linux_unittest.cc", "../browser/performance_manager/graph/policies/dynamic_tcmalloc_policy_linux_unittest.cc",
"../browser/performance_manager/graph/policies/high_pmf_memory_pressure_policy_unittest.cc",
"../browser/performance_manager/graph/policies/working_set_trimmer_policy_chromeos_unittest.cc", "../browser/performance_manager/graph/policies/working_set_trimmer_policy_chromeos_unittest.cc",
"../browser/performance_manager/graph/policies/working_set_trimmer_policy_unittest.cc", "../browser/performance_manager/graph/policies/working_set_trimmer_policy_unittest.cc",
"../browser/performance_manager/mechanisms/working_set_trimmer_win_unittest.cc", "../browser/performance_manager/mechanisms/working_set_trimmer_win_unittest.cc",
......
...@@ -19,6 +19,7 @@ ...@@ -19,6 +19,7 @@
#include "components/performance_manager/graph/system_node_impl.h" #include "components/performance_manager/graph/system_node_impl.h"
#include "components/performance_manager/graph/worker_node_impl.h" #include "components/performance_manager/graph/worker_node_impl.h"
#include "components/performance_manager/public/render_process_host_proxy.h" #include "components/performance_manager/public/render_process_host_proxy.h"
#include "content/public/test/browser_task_environment.h"
#include "testing/gtest/include/gtest/gtest.h" #include "testing/gtest/include/gtest/gtest.h"
namespace performance_manager { namespace performance_manager {
...@@ -193,7 +194,7 @@ class GraphTestHarness : public ::testing::Test { ...@@ -193,7 +194,7 @@ class GraphTestHarness : public ::testing::Test {
GraphTestHarness(); GraphTestHarness();
~GraphTestHarness() override; ~GraphTestHarness() override;
// Optional constructor for directly configuring the TaskEnvironment. // Optional constructor for directly configuring the BrowserTaskEnvironment.
template <class... ArgTypes> template <class... ArgTypes>
explicit GraphTestHarness(ArgTypes... args) : task_env_(args...) {} explicit GraphTestHarness(ArgTypes... args) : task_env_(args...) {}
...@@ -223,11 +224,11 @@ class GraphTestHarness : public ::testing::Test { ...@@ -223,11 +224,11 @@ class GraphTestHarness : public ::testing::Test {
protected: protected:
void AdvanceClock(base::TimeDelta delta) { task_env_.FastForwardBy(delta); } void AdvanceClock(base::TimeDelta delta) { task_env_.FastForwardBy(delta); }
base::test::TaskEnvironment& task_env() { return task_env_; } content::BrowserTaskEnvironment& task_env() { return task_env_; }
TestGraphImpl* graph() { return &graph_; } TestGraphImpl* graph() { return &graph_; }
private: private:
base::test::TaskEnvironment task_env_; content::BrowserTaskEnvironment task_env_;
TestGraphImpl graph_; TestGraphImpl graph_;
}; };
......
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