Skip to content

Commit

Permalink
Mojo: Small fixes/cleanup to MessageInTransit, before I factor out th…
Browse files Browse the repository at this point in the history 
…e 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
  • Loading branch information
viettrungluu@chromium.org committed May 1, 2014
1 parent 783fd12 commit fdfd21c
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 46 deletions.
73 changes: 38 additions & 35 deletions mojo/system/message_in_transit.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -106,9 +105,9 @@ MessageInTransit::MessageInTransit(Type type,
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);

Expand Down Expand Up @@ -136,26 +135,25 @@ MessageInTransit::MessageInTransit(Type type,
// 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_,
RoundUpMessageAlignment(sizeof(Header) + num_bytes()));
}

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])
Expand All @@ -172,10 +170,7 @@ MessageInTransit::~MessageInTransit() {
}

#ifndef NDEBUG
main_buffer_size_ = 0;
main_buffer_ = NULL;
secondary_buffer_size_ = 0;
secondary_buffer_ = NULL;
dispatchers_.reset();
platform_handles_.reset();
#endif
Expand Down Expand Up @@ -216,35 +211,35 @@ void MessageInTransit::SetDispatchers(
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.
// TODO(vtl): Add a header to the secondary buffer.
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;
Dispatcher::MessageInTransitAccess::StartSerialize(
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);
Expand All @@ -258,22 +253,22 @@ void MessageInTransit::SerializeAndCloseDispatchers(Channel* channel) {
}
}

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_);
platform_handles_.reset(new std::vector<embedder::PlatformHandle>());
}

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,
Expand All @@ -286,7 +281,7 @@ void MessageInTransit::SerializeAndCloseDispatchers(Channel* channel) {
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,
Expand All @@ -308,11 +303,19 @@ void MessageInTransit::SerializeAndCloseDispatchers(Channel* channel) {
}

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();
}

Expand All @@ -335,7 +338,7 @@ void MessageInTransit::DeserializeDispatchers(Channel* channel) {
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;
Expand All @@ -344,7 +347,7 @@ void MessageInTransit::DeserializeDispatchers(Channel* channel) {
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);
}
Expand Down
21 changes: 10 additions & 11 deletions mojo/system/message_in_transit.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -178,11 +179,11 @@ class MOJO_SYSTEM_IMPL_EXPORT MessageInTransit {
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).
Expand All @@ -192,10 +193,8 @@ class MOJO_SYSTEM_IMPL_EXPORT MessageInTransit {
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; }

Expand Down Expand Up @@ -281,17 +280,17 @@ class MOJO_SYSTEM_IMPL_EXPORT MessageInTransit {
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
Expand Down

0 comments on commit fdfd21c

Please sign in to comment.