Mojo: Small fixes/cleanup to MessageInTransit, before I factor out the secondary buffer.
I'm making these changes as I write the factored-out secondary buffer in
parallel, but it's probably to review these changes now rather than as a
part of the move/refactor.
Notable changes:
* Use the actual size for the secondary buffer, rather than the
estimated size. (Probably this makes no difference now.)
* Discard the dispatchers earlier (in SerializeAndCloseDispatchers()).
R=darin@chromium.org
Review URL: https://codereview.chromium.org/260823002
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@267542 0039d316-1c4b-4281-b951-d872f2087c98
CrOS-Libchrome-Original-Commit: fdfd21c22bd7c7f96e0fee37931eba5249ef572a
diff --git a/mojo/system/message_in_transit.cc b/mojo/system/message_in_transit.cc
index c15d862..a38ac1c 100644
--- a/mojo/system/message_in_transit.cc
+++ b/mojo/system/message_in_transit.cc
@@ -10,7 +10,6 @@
#include "base/compiler_specific.h"
#include "base/logging.h"
-#include "base/memory/aligned_memory.h"
#include "mojo/system/constants.h"
namespace mojo {
@@ -106,9 +105,9 @@
uint32_t num_handles,
const void* bytes)
: main_buffer_size_(RoundUpMessageAlignment(sizeof(Header) + num_bytes)),
- main_buffer_(base::AlignedAlloc(main_buffer_size_, kMessageAlignment)),
- secondary_buffer_size_(0),
- secondary_buffer_(NULL) {
+ main_buffer_(static_cast<char*>(base::AlignedAlloc(main_buffer_size_,
+ kMessageAlignment))),
+ secondary_buffer_size_(0) {
DCHECK_LE(num_bytes, kMaxMessageNumBytes);
DCHECK_LE(num_handles, kMaxMessageNumHandles);
@@ -136,16 +135,18 @@
// I just create (deserialize) the dispatchers right away?
MessageInTransit::MessageInTransit(const View& message_view)
: main_buffer_size_(message_view.main_buffer_size()),
- main_buffer_(base::AlignedAlloc(main_buffer_size_, kMessageAlignment)),
+ main_buffer_(static_cast<char*>(base::AlignedAlloc(main_buffer_size_,
+ kMessageAlignment))),
secondary_buffer_size_(message_view.secondary_buffer_size()),
secondary_buffer_(secondary_buffer_size_ ?
- base::AlignedAlloc(secondary_buffer_size_,
- kMessageAlignment) : NULL) {
+ static_cast<char*>(
+ base::AlignedAlloc(secondary_buffer_size_,
+ kMessageAlignment)) : NULL) {
DCHECK_GE(main_buffer_size_, sizeof(Header));
DCHECK_EQ(main_buffer_size_ % kMessageAlignment, 0u);
- memcpy(main_buffer_, message_view.main_buffer(), main_buffer_size_);
- memcpy(secondary_buffer_, message_view.secondary_buffer(),
+ memcpy(main_buffer_.get(), message_view.main_buffer(), main_buffer_size_);
+ memcpy(secondary_buffer_.get(), message_view.secondary_buffer(),
secondary_buffer_size_);
DCHECK_EQ(main_buffer_size_,
@@ -153,9 +154,6 @@
}
MessageInTransit::~MessageInTransit() {
- base::AlignedFree(main_buffer_);
- base::AlignedFree(secondary_buffer_); // Okay if null.
-
if (dispatchers_) {
for (size_t i = 0; i < dispatchers_->size(); i++) {
if (!(*dispatchers_)[i])
@@ -172,10 +170,7 @@
}
#ifndef NDEBUG
- main_buffer_size_ = 0;
- main_buffer_ = NULL;
secondary_buffer_size_ = 0;
- secondary_buffer_ = NULL;
dispatchers_.reset();
platform_handles_.reset();
#endif
@@ -216,10 +211,9 @@
void MessageInTransit::SerializeAndCloseDispatchers(Channel* channel) {
DCHECK(channel);
DCHECK(!secondary_buffer_);
- CHECK_EQ(num_handles(),
- dispatchers_ ? dispatchers_->size() : static_cast<size_t>(0));
- if (!num_handles())
+ const size_t num_handles = dispatchers_ ? dispatchers_->size() : 0;
+ if (!num_handles)
return;
// The offset to the start of the (Mojo) handle table.
@@ -227,15 +221,16 @@
const size_t handle_table_start_offset = 0;
// The offset to the start of the serialized dispatcher data.
const size_t serialized_dispatcher_start_offset =
- handle_table_start_offset + num_handles() * sizeof(HandleTableEntry);
- // The size of the secondary buffer we'll add to this as we go along).
- size_t size = serialized_dispatcher_start_offset;
+ handle_table_start_offset + num_handles * sizeof(HandleTableEntry);
+ // The estimated size of the secondary buffer. We compute this estimate below.
+ // It must be at least as big as the (eventual) actual size.
+ size_t estimated_size = serialized_dispatcher_start_offset;
size_t num_platform_handles = 0;
#if DCHECK_IS_ON
- std::vector<size_t> all_max_sizes(num_handles());
- std::vector<size_t> all_max_platform_handles(num_handles());
+ std::vector<size_t> all_max_sizes(num_handles);
+ std::vector<size_t> all_max_platform_handles(num_handles);
#endif
- for (size_t i = 0; i < num_handles(); i++) {
+ for (size_t i = 0; i < num_handles; i++) {
if (Dispatcher* dispatcher = (*dispatchers_)[i].get()) {
size_t max_size = 0;
size_t max_platform_handles = 0;
@@ -243,8 +238,8 @@
dispatcher, channel, &max_size, &max_platform_handles);
DCHECK_LE(max_size, kMaxSerializedDispatcherSize);
- size += RoundUpMessageAlignment(max_size);
- DCHECK_LE(size, kMaxSecondaryBufferSize);
+ estimated_size += RoundUpMessageAlignment(max_size);
+ DCHECK_LE(estimated_size, kMaxSecondaryBufferSize);
DCHECK_LE(max_platform_handles,
kMaxSerializedDispatcherPlatformHandles);
@@ -258,12 +253,12 @@
}
}
- secondary_buffer_ = base::AlignedAlloc(size, kMessageAlignment);
- secondary_buffer_size_ = static_cast<uint32_t>(size);
+ secondary_buffer_.reset(static_cast<char*>(
+ base::AlignedAlloc(estimated_size, kMessageAlignment)));
// Entirely clear out the secondary buffer, since then we won't have to worry
// about clearing padding or unused space (e.g., if a dispatcher fails to
// serialize).
- memset(secondary_buffer_, 0, size);
+ memset(secondary_buffer_.get(), 0, estimated_size);
if (num_platform_handles > 0) {
DCHECK(!platform_handles_);
@@ -271,9 +266,9 @@
}
HandleTableEntry* handle_table = reinterpret_cast<HandleTableEntry*>(
- static_cast<char*>(secondary_buffer_) + handle_table_start_offset);
+ secondary_buffer_.get() + handle_table_start_offset);
size_t current_offset = serialized_dispatcher_start_offset;
- for (size_t i = 0; i < num_handles(); i++) {
+ for (size_t i = 0; i < num_handles; i++) {
Dispatcher* dispatcher = (*dispatchers_)[i].get();
if (!dispatcher) {
COMPILE_ASSERT(Dispatcher::kTypeUnknown == 0,
@@ -286,7 +281,7 @@
platform_handles_ ? platform_handles_->size() : 0;
#endif
- void* destination = static_cast<char*>(secondary_buffer_) + current_offset;
+ void* destination = secondary_buffer_.get() + current_offset;
size_t actual_size = 0;
if (Dispatcher::MessageInTransitAccess::EndSerializeAndClose(
dispatcher, channel, destination, &actual_size,
@@ -308,11 +303,19 @@
}
current_offset += RoundUpMessageAlignment(actual_size);
- DCHECK_LE(current_offset, size);
+ DCHECK_LE(current_offset, estimated_size);
DCHECK_LE(platform_handles_ ? platform_handles_->size() : 0,
num_platform_handles);
}
+ // There's no aligned realloc, so it's no good way to release unused space (if
+ // we overshot our estimated space requirements).
+ secondary_buffer_size_ = current_offset;
+
+ // The dispatchers (which we "owned") were closed. We can dispose of them now.
+ dispatchers_.reset();
+
+ // Update the sizes in the message header.
UpdateTotalSize();
}
@@ -335,7 +338,7 @@
DCHECK_LE(handle_table_size, secondary_buffer_size_);
const HandleTableEntry* handle_table =
- static_cast<const HandleTableEntry*>(secondary_buffer_);
+ reinterpret_cast<const HandleTableEntry*>(secondary_buffer_.get());
for (size_t i = 0; i < num_handles(); i++) {
size_t offset = handle_table[i].offset;
size_t size = handle_table[i].size;
@@ -344,7 +347,7 @@
DCHECK_LE(offset, secondary_buffer_size_);
DCHECK_LE(offset + size, secondary_buffer_size_);
- const void* source = static_cast<const char*>(secondary_buffer_) + offset;
+ const void* source = secondary_buffer_.get() + offset;
(*dispatchers_)[i] = Dispatcher::MessageInTransitAccess::Deserialize(
channel, handle_table[i].type, source, size);
}
diff --git a/mojo/system/message_in_transit.h b/mojo/system/message_in_transit.h
index 4018dd6..1fc021e 100644
--- a/mojo/system/message_in_transit.h
+++ b/mojo/system/message_in_transit.h
@@ -10,6 +10,7 @@
#include <vector>
#include "base/macros.h"
+#include "base/memory/aligned_memory.h"
#include "base/memory/scoped_ptr.h"
#include "mojo/embedder/platform_handle.h"
#include "mojo/system/dispatcher.h"
@@ -178,11 +179,11 @@
void DeserializeDispatchers(Channel* channel);
// Gets the main buffer and its size (in number of bytes), respectively.
- const void* main_buffer() const { return main_buffer_; }
+ const void* main_buffer() const { return main_buffer_.get(); }
size_t main_buffer_size() const { return main_buffer_size_; }
// Gets the secondary buffer and its size (in number of bytes), respectively.
- const void* secondary_buffer() const { return secondary_buffer_; }
+ const void* secondary_buffer() const { return secondary_buffer_.get(); }
size_t secondary_buffer_size() const { return secondary_buffer_size_; }
// Gets the total size of the message (see comment in |Header|, below).
@@ -192,10 +193,8 @@
uint32_t num_bytes() const { return header()->num_bytes; }
// Gets the message data (of size |num_bytes()| bytes).
- const void* bytes() const {
- return static_cast<const char*>(main_buffer_) + sizeof(Header);
- }
- void* bytes() { return static_cast<char*>(main_buffer_) + sizeof(Header); }
+ const void* bytes() const { return main_buffer_.get() + sizeof(Header); }
+ void* bytes() { return main_buffer_.get() + sizeof(Header); }
uint32_t num_handles() const { return header()->num_handles; }
@@ -281,17 +280,17 @@
size_t secondary_buffer_size);
const Header* header() const {
- return static_cast<const Header*>(main_buffer_);
+ return reinterpret_cast<const Header*>(main_buffer_.get());
}
- Header* header() { return static_cast<Header*>(main_buffer_); }
+ Header* header() { return reinterpret_cast<Header*>(main_buffer_.get()); }
void UpdateTotalSize();
- size_t main_buffer_size_;
- void* main_buffer_;
+ const size_t main_buffer_size_;
+ const scoped_ptr<char, base::AlignedFreeDeleter> main_buffer_; // Never null.
size_t secondary_buffer_size_;
- void* secondary_buffer_; // May be null.
+ scoped_ptr<char, base::AlignedFreeDeleter> secondary_buffer_; // May be null.
// Any dispatchers that may be attached to this message. These dispatchers
// should be "owned" by this message, i.e., have a ref count of exactly 1. (We