0

Revert raw_ptr rewrites based on sampling profiler data

Sampling profiler data revealed some raw_ptr in performance sensitive code.

Analysis: 
https: //docs.google.com/spreadsheets/d/18l_zAaKE1OaRfcPs8kL9ef7dyyGfh3oO4mh5LoytKYo/edit?resourcekey=0--oPpC5b7ENQw5ZjgB5hvfw#gid=1069036117
Bug: 1287151
Change-Id: Id24e4850221468fbdf413dc47ba2f4ca9b1855d7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3372102
Reviewed-by: Joe DeBlasio <jdeblasio@chromium.org>
Reviewed-by: Dominic Farolino <dom@chromium.org>
Reviewed-by: Bartek Nowierski <bartekn@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: danakj chromium <danakj@chromium.org>
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Commit-Queue: Keishi Hattori <keishi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#960441}
This commit is contained in:
Keishi Hattori
2022-01-18 16:48:45 +00:00
committed by Chromium LUCI CQ
parent c509d3afaa
commit 7ccb88c455
14 changed files with 115 additions and 44 deletions

@ -14,7 +14,6 @@
#include "base/check_op.h" #include "base/check_op.h"
#include "base/containers/span.h" #include "base/containers/span.h"
#include "base/gtest_prod_util.h" #include "base/gtest_prod_util.h"
#include "base/memory/raw_ptr.h"
#include "base/memory/ref_counted.h" #include "base/memory/ref_counted.h"
#include "base/strings/string_piece.h" #include "base/strings/string_piece.h"
@ -255,7 +254,7 @@ class BASE_EXPORT Pickle {
} }
const char* payload() const { const char* payload() const {
return reinterpret_cast<const char*>(header_.get()) + header_size_; return reinterpret_cast<const char*>(header_) + header_size_;
} }
// Returns the address of the byte immediately following the currently valid // Returns the address of the byte immediately following the currently valid
@ -271,7 +270,7 @@ class BASE_EXPORT Pickle {
size_t header_size() const { return header_size_; } size_t header_size() const { return header_size_; }
char* mutable_payload() { char* mutable_payload() {
return reinterpret_cast<char*>(header_.get()) + header_size_; return reinterpret_cast<char*>(header_) + header_size_;
} }
size_t capacity_after_header() const { size_t capacity_after_header() const {
@ -313,7 +312,9 @@ class BASE_EXPORT Pickle {
private: private:
friend class PickleIterator; friend class PickleIterator;
raw_ptr<Header> header_; // `header_` is not a raw_ptr<...> for performance reasons (based on analysis
// of sampling profiler data).
Header* header_;
size_t header_size_; // Supports extra data between header and payload. size_t header_size_; // Supports extra data between header and payload.
// Allocation size of payload (or -1 if allocation is const). Note: this // Allocation size of payload (or -1 if allocation is const). Note: this
// doesn't count the header. // doesn't count the header.

@ -135,7 +135,9 @@ class BASE_EXPORT TimerBase {
// Detects when the scheduled task is deleted before being executed. Null when // Detects when the scheduled task is deleted before being executed. Null when
// there is no scheduled task. // there is no scheduled task.
raw_ptr<TaskDestructionDetector> task_destruction_detector_ // `task_destruction_detector_` is not a raw_ptr<...> for performance reasons
// (based on analysis of sampling profiler data).
TaskDestructionDetector* task_destruction_detector_
GUARDED_BY_CONTEXT(sequence_checker_); GUARDED_BY_CONTEXT(sequence_checker_);
// If true, |user_task_| is scheduled to run sometime in the future. // If true, |user_task_| is scheduled to run sometime in the future.

@ -8,7 +8,6 @@
#include <stddef.h> #include <stddef.h>
#include "base/containers/stack_container.h" #include "base/containers/stack_container.h"
#include "base/memory/raw_ptr.h"
#include "cc/cc_export.h" #include "cc/cc_export.h"
#include "cc/tiles/picture_layer_tiling_set.h" #include "cc/tiles/picture_layer_tiling_set.h"
#include "cc/tiles/prioritized_tile.h" #include "cc/tiles/prioritized_tile.h"
@ -195,7 +194,9 @@ class CC_EXPORT TilingSetRasterQueueAll {
void MakeTilingIterator(IteratorType type, PictureLayerTiling* tiling); void MakeTilingIterator(IteratorType type, PictureLayerTiling* tiling);
void AdvanceToNextStage(); void AdvanceToNextStage();
raw_ptr<PictureLayerTilingSet> tiling_set_; // `tiling_set_` is not a raw_ptr<...> for performance reasons (based on
// analysis of sampling profiler data).
PictureLayerTilingSet* tiling_set_;
struct IterationStage { struct IterationStage {
IterationStage(IteratorType type, TilePriority::PriorityBin bin); IterationStage(IteratorType type, TilePriority::PriorityBin bin);

@ -11,7 +11,6 @@
#include <utility> #include <utility>
#include "base/gtest_prod_util.h" #include "base/gtest_prod_util.h"
#include "base/memory/raw_ptr.h"
#include "base/time/time.h" #include "base/time/time.h"
#include "base/values.h" #include "base/values.h"
#include "components/site_engagement/core/mojom/site_engagement_details.mojom-forward.h" #include "components/site_engagement/core/mojom/site_engagement_details.mojom-forward.h"
@ -215,7 +214,9 @@ class SiteEngagementScore {
// The clock used to vend times. Enables time travelling in tests. Owned by // The clock used to vend times. Enables time travelling in tests. Owned by
// the SiteEngagementService. // the SiteEngagementService.
raw_ptr<base::Clock> clock_; // `clock_` is not a raw_ptr<...> for performance reasons (based on analysis
// of sampling profiler data).
base::Clock* clock_;
// |raw_score_| is the score before any decay is applied. // |raw_score_| is the score before any decay is applied.
double raw_score_; double raw_score_;
@ -240,7 +241,9 @@ class SiteEngagementScore {
GURL origin_; GURL origin_;
// The settings to write this score to when Commit() is called. // The settings to write this score to when Commit() is called.
raw_ptr<HostContentSettingsMap> settings_map_; // `settings_map_` is not a raw_ptr<...> for performance reasons (based on
// analysis of sampling profiler data).
HostContentSettingsMap* settings_map_;
}; };
} // namespace site_engagement } // namespace site_engagement

@ -10,6 +10,7 @@
#include <string> #include <string>
#include <vector> #include <vector>
#include "base/memory/raw_ptr.h"
#include "content/common/content_export.h" #include "content/common/content_export.h"
#include "third_party/abseil-cpp/absl/types/optional.h" #include "third_party/abseil-cpp/absl/types/optional.h"
#include "url/gurl.h" #include "url/gurl.h"

@ -6,7 +6,6 @@
#define EXTENSIONS_BROWSER_LAZY_CONTEXT_TASK_QUEUE_H_ #define EXTENSIONS_BROWSER_LAZY_CONTEXT_TASK_QUEUE_H_
#include "base/callback.h" #include "base/callback.h"
#include "base/memory/raw_ptr.h"
#include "extensions/common/extension_id.h" #include "extensions/common/extension_id.h"
#include "url/gurl.h" #include "url/gurl.h"
@ -32,16 +31,22 @@ class LazyContextTaskQueue {
// consumers that add tasks to LazyContextTaskQueue. // consumers that add tasks to LazyContextTaskQueue.
struct ContextInfo { struct ContextInfo {
const ExtensionId extension_id; const ExtensionId extension_id;
const raw_ptr<content::RenderProcessHost> render_process_host; // `render_process_host` is not a raw_ptr<...> for performance reasons
// (based on analysis of sampling profiler data).
content::RenderProcessHost* const render_process_host;
const int64_t service_worker_version_id; const int64_t service_worker_version_id;
const int worker_thread_id; const int worker_thread_id;
const GURL url; const GURL url;
// TODO(dbertoni): This needs to be initialized for the Service Worker // TODO(dbertoni): This needs to be initialized for the Service Worker
// version of the constructor. // version of the constructor.
const raw_ptr<content::BrowserContext> browser_context = nullptr; // `browser_context` is not a raw_ptr<...> for performance reasons (based on
// analysis of sampling profiler data).
content::BrowserContext* const browser_context = nullptr;
// This data member will have a nullptr value for Service Worker-related // This data member will have a nullptr value for Service Worker-related
// tasks. // tasks.
const raw_ptr<content::WebContents> web_contents = nullptr; // `web_contents` is not a raw_ptr<...> for performance reasons (based on
// analysis of sampling profiler data).
content::WebContents* const web_contents = nullptr;
explicit ContextInfo(ExtensionHost* host); explicit ContextInfo(ExtensionHost* host);

@ -10,7 +10,6 @@
#include <memory> #include <memory>
#include <queue> #include <queue>
#include "base/memory/raw_ptr.h"
#include "mojo/core/atomic_flag.h" #include "mojo/core/atomic_flag.h"
#include "mojo/core/dispatcher.h" #include "mojo/core/dispatcher.h"
#include "mojo/core/ports/port_ref.h" #include "mojo/core/ports/port_ref.h"
@ -92,7 +91,9 @@ class MessagePipeDispatcher : public Dispatcher {
void OnPortStatusChanged(); void OnPortStatusChanged();
// These are safe to access from any thread without locking. // These are safe to access from any thread without locking.
const raw_ptr<NodeController> node_controller_; // `node_controller_` is not a raw_ptr<...> for performance reasons (based on
// analysis of sampling profiler data).
NodeController* const node_controller_;
const ports::PortRef port_; const ports::PortRef port_;
const uint64_t pipe_id_; const uint64_t pipe_id_;
const int endpoint_; const int endpoint_;

@ -7,7 +7,6 @@
#include <type_traits> #include <type_traits>
#include "base/memory/raw_ptr.h"
#include "mojo/public/cpp/bindings/lib/array_internal.h" #include "mojo/public/cpp/bindings/lib/array_internal.h"
#include "mojo/public/cpp/bindings/lib/bindings_internal.h" #include "mojo/public/cpp/bindings/lib/bindings_internal.h"
#include "mojo/public/cpp/bindings/lib/serialization_forward.h" #include "mojo/public/cpp/bindings/lib/serialization_forward.h"
@ -37,8 +36,12 @@ class ArrayDataViewImpl<
const T* data() const { return data_->storage(); } const T* data() const { return data_->storage(); }
protected: protected:
raw_ptr<Data_> data_; // `data_` is not a raw_ptr<...> for performance reasons (based on analysis of
raw_ptr<Message> message_; // sampling profiler data).
Data_* data_;
// `message_` is not a raw_ptr<...> for performance reasons (based on analysis
// of sampling profiler data).
Message* message_;
}; };
template <typename T> template <typename T>
@ -55,8 +58,12 @@ class ArrayDataViewImpl<
bool operator[](size_t index) const { return data_->at(index); } bool operator[](size_t index) const { return data_->at(index); }
protected: protected:
raw_ptr<Data_> data_; // `data_` is not a raw_ptr<...> for performance reasons (based on analysis of
raw_ptr<Message> message_; // sampling profiler data).
Data_* data_;
// `message_` is not a raw_ptr<...> for performance reasons (based on analysis
// of sampling profiler data).
Message* message_;
}; };
template <typename T> template <typename T>
@ -83,8 +90,12 @@ class ArrayDataViewImpl<
} }
protected: protected:
raw_ptr<Data_> data_; // `data_` is not a raw_ptr<...> for performance reasons (based on analysis of
raw_ptr<Message> message_; // sampling profiler data).
Data_* data_;
// `message_` is not a raw_ptr<...> for performance reasons (based on analysis
// of sampling profiler data).
Message* message_;
}; };
template <typename T> template <typename T>
@ -111,8 +122,12 @@ class ArrayDataViewImpl<
} }
protected: protected:
raw_ptr<Data_> data_; // `data_` is not a raw_ptr<...> for performance reasons (based on analysis of
raw_ptr<Message> message_; // sampling profiler data).
Data_* data_;
// `message_` is not a raw_ptr<...> for performance reasons (based on analysis
// of sampling profiler data).
Message* message_;
}; };
template <typename T> template <typename T>
@ -134,8 +149,12 @@ class ArrayDataViewImpl<
} }
protected: protected:
raw_ptr<Data_> data_; // `data_` is not a raw_ptr<...> for performance reasons (based on analysis of
raw_ptr<Message> message_; // sampling profiler data).
Data_* data_;
// `message_` is not a raw_ptr<...> for performance reasons (based on analysis
// of sampling profiler data).
Message* message_;
}; };
template <typename T> template <typename T>
@ -162,8 +181,12 @@ class ArrayDataViewImpl<
} }
protected: protected:
raw_ptr<Data_> data_; // `data_` is not a raw_ptr<...> for performance reasons (based on analysis of
raw_ptr<Message> message_; // sampling profiler data).
Data_* data_;
// `message_` is not a raw_ptr<...> for performance reasons (based on analysis
// of sampling profiler data).
Message* message_;
}; };
template <typename T> template <typename T>
@ -187,8 +210,12 @@ class ArrayDataViewImpl<
} }
protected: protected:
raw_ptr<Data_> data_; // `data_` is not a raw_ptr<...> for performance reasons (based on analysis of
raw_ptr<Message> message_; // sampling profiler data).
Data_* data_;
// `message_` is not a raw_ptr<...> for performance reasons (based on analysis
// of sampling profiler data).
Message* message_;
}; };
} // namespace internal } // namespace internal

@ -12,7 +12,6 @@
#include "base/callback.h" #include "base/callback.h"
#include "base/compiler_specific.h" #include "base/compiler_specific.h"
#include "base/component_export.h" #include "base/component_export.h"
#include "base/memory/raw_ptr.h"
#include "base/memory/ref_counted.h" #include "base/memory/ref_counted.h"
#include "base/memory/weak_ptr.h" #include "base/memory/weak_ptr.h"
#include "base/sequence_checker.h" #include "base/sequence_checker.h"
@ -296,7 +295,9 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS) Connector : public MessageReceiver {
base::OnceClosure connection_error_handler_; base::OnceClosure connection_error_handler_;
ScopedMessagePipeHandle message_pipe_; ScopedMessagePipeHandle message_pipe_;
raw_ptr<MessageReceiver> incoming_receiver_ = nullptr; // `incoming_receiver_` is not a raw_ptr<...> for performance reasons (based
// on analysis of sampling profiler data).
MessageReceiver* incoming_receiver_ = nullptr;
scoped_refptr<base::SequencedTaskRunner> task_runner_; scoped_refptr<base::SequencedTaskRunner> task_runner_;
std::unique_ptr<SimpleWatcher> handle_watcher_; std::unique_ptr<SimpleWatcher> handle_watcher_;
@ -341,7 +342,9 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS) Connector : public MessageReceiver {
// A cached pointer to the RunLoopNestingObserver for the thread on which this // A cached pointer to the RunLoopNestingObserver for the thread on which this
// Connector was created. // Connector was created.
raw_ptr<RunLoopNestingObserver> nesting_observer_ = nullptr; // `nesting_observer_` is not a raw_ptr<...> for performance reasons (based on
// analysis of sampling profiler data).
RunLoopNestingObserver* nesting_observer_ = nullptr;
// |true| iff the Connector is currently dispatching a message. Used to detect // |true| iff the Connector is currently dispatching a message. Used to detect
// nested dispatch operations. // nested dispatch operations.

@ -293,10 +293,13 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS) InterfaceEndpointClient
ScopedInterfaceEndpointHandle handle_; ScopedInterfaceEndpointHandle handle_;
std::unique_ptr<AssociatedGroup> associated_group_; std::unique_ptr<AssociatedGroup> associated_group_;
raw_ptr<InterfaceEndpointController> controller_ = nullptr; // `controller_` is not a raw_ptr<...> for performance reasons (based on
// analysis of sampling profiler data).
InterfaceEndpointController* controller_ = nullptr;
const raw_ptr<MessageReceiverWithResponderStatus> incoming_receiver_ = // `incoming_receiver_` is not a raw_ptr<...> for performance reasons (based
nullptr; // on analysis of sampling profiler data).
MessageReceiverWithResponderStatus* const incoming_receiver_ = nullptr;
HandleIncomingMessageThunk thunk_{this}; HandleIncomingMessageThunk thunk_{this};
MessageDispatcher dispatcher_; MessageDispatcher dispatcher_;

@ -6,7 +6,6 @@
#define MOJO_PUBLIC_CPP_BINDINGS_LIB_MAY_AUTO_LOCK_H_ #define MOJO_PUBLIC_CPP_BINDINGS_LIB_MAY_AUTO_LOCK_H_
#include "base/component_export.h" #include "base/component_export.h"
#include "base/memory/raw_ptr.h"
#include "base/synchronization/lock.h" #include "base/synchronization/lock.h"
#include "third_party/abseil-cpp/absl/types/optional.h" #include "third_party/abseil-cpp/absl/types/optional.h"
@ -34,7 +33,9 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS_BASE) MayAutoLock {
} }
private: private:
raw_ptr<base::Lock> lock_; // `lock_` is not a raw_ptr<...> for performance reasons: on-stack pointer +
// based on analysis of sampling profiler data.
base::Lock* lock_;
}; };
// Similar to base::AutoUnlock, except that it does nothing if |lock| passed // Similar to base::AutoUnlock, except that it does nothing if |lock| passed
@ -58,7 +59,9 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS_BASE) MayAutoUnlock {
} }
private: private:
raw_ptr<base::Lock> lock_; // `lock_` is not a raw_ptr<...> for performance reasons: on-stack pointer +
// based on analysis of sampling profiler data.
base::Lock* lock_;
}; };
} // namespace internal } // namespace internal

@ -14,7 +14,6 @@
#include "base/component_export.h" #include "base/component_export.h"
#include "base/containers/circular_deque.h" #include "base/containers/circular_deque.h"
#include "base/containers/small_map.h" #include "base/containers/small_map.h"
#include "base/memory/raw_ptr.h"
#include "base/memory/scoped_refptr.h" #include "base/memory/scoped_refptr.h"
#include "base/sequence_checker.h" #include "base/sequence_checker.h"
#include "base/synchronization/lock.h" #include "base/synchronization/lock.h"
@ -305,7 +304,9 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS) MultiplexRouter
scoped_refptr<base::SequencedTaskRunner> task_runner_; scoped_refptr<base::SequencedTaskRunner> task_runner_;
// Owned by |dispatcher_| below. // Owned by |dispatcher_| below.
raw_ptr<MessageHeaderValidator> header_validator_ = nullptr; // `header_validator_` is not a raw_ptr<...> for performance reasons (based on
// analysis of sampling profiler data).
MessageHeaderValidator* header_validator_ = nullptr;
MessageDispatcher dispatcher_; MessageDispatcher dispatcher_;
Connector connector_; Connector connector_;

@ -342,6 +342,25 @@ mojo::internal::MultiplexRouter::MessageWrapper::router_
tracing::(anonymous namespace)::LazyLegacyEventInitializer::legacy_event_ tracing::(anonymous namespace)::LazyLegacyEventInitializer::legacy_event_
tracing::(anonymous namespace)::LazyLegacyEventInitializer::track_event_ tracing::(anonymous namespace)::LazyLegacyEventInitializer::track_event_
url::StdStringCanonOutput::str_ url::StdStringCanonOutput::str_
base::Pickle::header_
base::internal::TimerBase::task_destruction_detector_
cc::TilingSetRasterQueueAll::tiling_set_
site_engagement::SiteEngagementScore::clock_
site_engagement::SiteEngagementScore::settings_map_
extensions::LazyContextTaskQueue::ContextInfo::render_process_host
extensions::LazyContextTaskQueue::ContextInfo::browser_context
extensions::LazyContextTaskQueue::ContextInfo::web_contents
mojo::core::MessagePipeDispatcher::node_controller_
mojo::internal::ArrayDataViewImpl::data_
mojo::internal::ArrayDataViewImpl::message_
mojo::Connector::incoming_receiver_
mojo::Connector::nesting_observer_
mojo::InterfaceEndpointClient::controller_
mojo::InterfaceEndpointClient::incoming_receiver_
mojo::internal::MayAutoLock::lock_
mojo::internal::MayAutoUnlock::lock_
mojo::internal::MultiplexRouter::header_validator_
url::CanonOutputT::buffer_
# Populated manually - type is unsupported by raw_ptr to avoid being used in # Populated manually - type is unsupported by raw_ptr to avoid being used in
# performance sensitive base::Unretained # performance sensitive base::Unretained

@ -12,7 +12,6 @@
#include "base/component_export.h" #include "base/component_export.h"
#include "base/export_template.h" #include "base/export_template.h"
#include "base/memory/raw_ptr.h"
#include "url/third_party/mozilla/url_parse.h" #include "url/third_party/mozilla/url_parse.h"
namespace url { namespace url {
@ -140,7 +139,9 @@ class CanonOutputT {
return true; return true;
} }
raw_ptr<T> buffer_; // `buffer_` is not a raw_ptr<...> for performance reasons (based on analysis
// of sampling profiler data).
T* buffer_;
int buffer_len_; int buffer_len_;
// Used characters in the buffer. // Used characters in the buffer.