From 96b39bb6bece88430ef05cafccdb922736fc7bd7 Mon Sep 17 00:00:00 2001 From: Mike Kaufman Date: Fri, 25 Aug 2017 12:45:00 -0700 Subject: [PATCH 1/4] http2, async-wrap: introduce AliasedBuffer class This change introduces an AliasedBuffer class and updates asytnc-wrap and http2 to use this class. A common technique to optimize performance is to create a native buffer and then map that native buffer to user space via JS array. The runtime can efficiently write to the native buffer without having to route though JS, and the values being written are accessible from user space. While efficient, this technique allows modifications to user space memory w/out going through JS type system APIs, effectively bypassing any monitoring the JS VM has in place to track program state modifications. The result is that monitors have an incorrect view of prorgram state. The AliasedBuffer class provides a future placeholder where this technique can be used, but writes can still be observed. To achieve this, the node-chakra-core fork will add in appropriate tracking logic in the AliasedBuffer's SetValue() method. Going forward, this class can evolve to support more sophisticated mechanisms if necessary. --- node.gyp | 4 + src/aliased_buffer.h | 194 +++++++++++++++++++++++++ src/async-wrap.cc | 16 +-- src/env-inl.h | 32 +++-- src/env.h | 24 ++-- src/node_http2.cc | 119 +++++----------- src/node_http2.h | 1 + src/node_http2_state.h | 116 +++++++++++++++ test/cctest/node_test_fixture.cc | 3 + test/cctest/node_test_fixture.h | 2 +- test/cctest/test_aliased_buffer.cc | 220 +++++++++++++++++++++++++++++ 11 files changed, 608 insertions(+), 123 deletions(-) create mode 100644 src/aliased_buffer.h create mode 100755 src/node_http2_state.h create mode 100644 test/cctest/node_test_fixture.cc create mode 100644 test/cctest/test_aliased_buffer.cc diff --git a/node.gyp b/node.gyp index 79d9e0a68dcedf..17f23564147dd1 100644 --- a/node.gyp +++ b/node.gyp @@ -228,6 +228,7 @@ 'src/util.cc', 'src/uv.cc', # headers to make for a more pleasant IDE experience + 'src/aliased_buffer.h', 'src/async-wrap.h', 'src/async-wrap-inl.h', 'src/base-object.h', @@ -246,6 +247,7 @@ 'src/node_constants.h', 'src/node_debug_options.h', 'src/node_http2.h', + 'src/node_http2_state.h', 'src/node_internals.h', 'src/node_javascript.h', 'src/node_mutex.h', @@ -650,6 +652,8 @@ 'sources': [ 'src/node_platform.cc', 'src/node_platform.h', + 'test/cctest/node_test_fixture.cc', + 'test/cctest/test_aliased_buffer.cc', 'test/cctest/test_base64.cc', 'test/cctest/test_environment.cc', 'test/cctest/test_util.cc', diff --git a/src/aliased_buffer.h b/src/aliased_buffer.h new file mode 100644 index 00000000000000..480c3a9f42815f --- /dev/null +++ b/src/aliased_buffer.h @@ -0,0 +1,194 @@ + +#ifndef SRC_ALIASED_BUFFER_H_ +#define SRC_ALIASED_BUFFER_H_ + +#include "v8.h" +#include "util.h" +#include "util-inl.h" + +namespace node { + +/** + * This class encapsulates the technique of having a native buffer mapped to + * a JS object. Writes to the native buffer can happen efficiently without + * going through JS, and the data is then available to user's via the exposed + * JS object. + * + * While this technique is computationaly efficient, it is effectively a + * write to JS program state w/out going through the standard + * (monitored) API. Thus any VM capabilities to detect the modification are + * circumvented. + * + * The encapsulation herein provides a placeholder where such writes can be + * observed. Any notification APIs will be left as a future exercise. + */ +template +class AliasedBuffer { + public: + AliasedBuffer(v8::Isolate* isolate, const size_t count) : + isolate_(isolate), + count_(count), + byte_offset_(0), + freeBuffer_(true) { + const v8::HandleScope handle_scope(this->isolate_); + + const size_t sizeInBytes = sizeof(NativeT) * count; + + // allocate native buffer + this->buffer_ = UncheckedCalloc(count); + if (this->buffer_ == nullptr) { + ABORT(); + } + + // allocate v8 ArrayBuffer + v8::Local ab = v8::ArrayBuffer::New( + this->isolate_, this->buffer_, sizeInBytes); + + // allocate v8 TypedArray + v8::Local jsArray = V8T::New(ab, this->byte_offset_, count); + this->jsArray_ = v8::Global(isolate, jsArray); + } + + /** + * Create an AliasedBuffer over a sub-region of another aliased buffer. + * The two will share a v8::ArrayBuffer instance & + * a native buffer, but will each read/write to different sections of the + * native buffer. + * + * Note that byte_offset must by aligned by sizeof(NativeT). + */ + AliasedBuffer( + v8::Isolate* isolate, + const size_t byte_offset, + const size_t count, + const AliasedBuffer& backingBuffer) : + isolate_(isolate), + count_(count), + byte_offset_(byte_offset), + freeBuffer_(false) { + const v8::HandleScope handle_scope(this->isolate_); + // validate that the byte_offset is aligned with sizeof(NativeT) + CHECK_EQ(byte_offset & (sizeof(NativeT) - 1), 0); + this->buffer_ = reinterpret_cast( + const_cast(backingBuffer.GetNativeBuffer() + byte_offset)); + + v8::Local ab = backingBuffer.GetArrayBuffer(); + v8::Local jsArray = V8T::New(ab, byte_offset, count); + this->jsArray_ = v8::Global(isolate, jsArray); + } + + AliasedBuffer(const AliasedBuffer& that) : + isolate_(that.isolate_), + count_(that.count_), + byte_offset_(that.byte_offset_), + buffer_(that.buffer_), + freeBuffer_(false) { + this->jsArray_ = v8::Global(that.isolate_, that.GetJSArray()); + } + + ~AliasedBuffer() { + if (this->freeBuffer_ && this->buffer_ != NULL) { + free(this->buffer_); + } + this->jsArray_.Reset(); + } + + /** + * Helper class that is returned from operator[] to support assignment into + * a specified location. + */ + class Reference { + public: + Reference(AliasedBuffer* aliasedBuffer, size_t index) : + aliasedBuffer_(aliasedBuffer), + index_(index) { + } + + Reference(const Reference& that) : + aliasedBuffer_(that.aliasedBuffer_), + index_(that.index_) { + } + + inline Reference& operator=(const NativeT &val) { + this->aliasedBuffer_->SetValue(this->index_, val); + return *this; + } + + operator NativeT() { + return this->aliasedBuffer_->GetValue(this->index_); + } + + private: + AliasedBuffer* aliasedBuffer_; + size_t index_; + }; + + /** + * Get the underlying v8 TypedArray overlayed on top of the native buffer + */ + v8::Local GetJSArray() const { + return this->jsArray_.Get(this->isolate_); + } + + /** + * Get the underlying v8::ArrayBuffer underlying the TypedArray and + * overlaying the native buffer + */ + v8::Local GetArrayBuffer() const { + return this->GetJSArray()->Buffer(); + } + + /** + * Get the underlying native buffer. Note that all reads/writes should occur + * through the GetValue/SetValue/operator[] methods + */ + inline const NativeT* GetNativeBuffer() const { + return this->buffer_; + } + + /** + * Synonym for GetBuffer() + */ + inline const NativeT* operator * () { + return this->GetNativeBuffer(); + } + + /** + * Set position index to given value. + */ + inline void SetValue(const size_t index, NativeT value) { + CHECK_LT(index, this->count_); + this->buffer_[index] = value; + } + + /** + * Get value at position index + */ + inline const NativeT GetValue(const size_t index) const { + CHECK_LT(index, this->count_); + return this->buffer_[index]; + } + + /** + * Effectively, a synonym for GetValue/SetValue + */ + Reference operator[](size_t index) { + return Reference(this, index); + } + + NativeT operator[](size_t index) const { + return this->GetValue(index); + } + + private: + v8::Isolate* const isolate_; + size_t count_; + size_t byte_offset_; + NativeT* buffer_; + v8::Global jsArray_; + bool freeBuffer_; +}; +} // namespace node + +#endif // SRC_ALIASED_BUFFER_H_ diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 9c25c518537bf9..391a684759cd03 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -31,7 +31,6 @@ #include "v8-profiler.h" using v8::Array; -using v8::ArrayBuffer; using v8::Context; using v8::Float64Array; using v8::Function; @@ -53,7 +52,6 @@ using v8::RetainedObjectInfo; using v8::String; using v8::Symbol; using v8::TryCatch; -using v8::Uint32Array; using v8::Undefined; using v8::Value; @@ -512,13 +510,9 @@ void AsyncWrap::Initialize(Local target, // callbacks waiting to be called on a particular event. It can then be // incremented/decremented from JS quickly to communicate to C++ if there are // any callbacks waiting to be called. - uint32_t* fields_ptr = env->async_hooks()->fields(); - int fields_count = env->async_hooks()->fields_count(); - Local fields_ab = - ArrayBuffer::New(isolate, fields_ptr, fields_count * sizeof(*fields_ptr)); FORCE_SET_TARGET_FIELD(target, "async_hook_fields", - Uint32Array::New(fields_ab, 0, fields_count)); + env->async_hooks()->fields().GetJSArray()); // The following v8::Float64Array has 5 fields. These fields are shared in // this way to allow JS and C++ to read/write each value as quickly as @@ -529,15 +523,9 @@ void AsyncWrap::Initialize(Local target, // kInitTriggerId: Write the id of the resource responsible for a handle's // creation just before calling the new handle's constructor. After the new // handle is constructed kInitTriggerId is set back to 0. - double* uid_fields_ptr = env->async_hooks()->uid_fields(); - int uid_fields_count = env->async_hooks()->uid_fields_count(); - Local uid_fields_ab = ArrayBuffer::New( - isolate, - uid_fields_ptr, - uid_fields_count * sizeof(*uid_fields_ptr)); FORCE_SET_TARGET_FIELD(target, "async_uid_fields", - Float64Array::New(uid_fields_ab, 0, uid_fields_count)); + env->async_hooks()->uid_fields().GetJSArray()); Local constants = Object::New(isolate); #define SET_HOOKS_CONSTANT(name) \ diff --git a/src/env-inl.h b/src/env-inl.h index d31b3602e97e79..6a2fca1284ae39 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -24,6 +24,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include "aliased_buffer.h" #include "env.h" #include "node.h" #include "util.h" @@ -82,8 +83,8 @@ inline uint32_t* IsolateData::zero_fill_field() const { inline Environment::AsyncHooks::AsyncHooks(v8::Isolate* isolate) : isolate_(isolate), - fields_(), - uid_fields_() { + fields_(isolate, kFieldsCount), + uid_fields_(isolate, kUidFieldsCount) { v8::HandleScope handle_scope(isolate_); // kAsyncUidCntr should start at 1 because that'll be the id the execution @@ -105,7 +106,8 @@ inline Environment::AsyncHooks::AsyncHooks(v8::Isolate* isolate) #undef V } -inline uint32_t* Environment::AsyncHooks::fields() { +inline AliasedBuffer& +Environment::AsyncHooks::fields() { return fields_; } @@ -113,7 +115,8 @@ inline int Environment::AsyncHooks::fields_count() const { return kFieldsCount; } -inline double* Environment::AsyncHooks::uid_fields() { +inline AliasedBuffer& +Environment::AsyncHooks::uid_fields() { return uid_fields_; } @@ -147,7 +150,7 @@ inline bool Environment::AsyncHooks::pop_ids(double async_id) { fprintf(stderr, "Error: async hook stack has become corrupted (" "actual: %.f, expected: %.f)\n", - uid_fields_[kCurrentAsyncId], + uid_fields_.GetValue(kCurrentAsyncId), async_id); Environment* env = Environment::GetCurrent(isolate_); DumpBacktrace(stderr); @@ -346,7 +349,7 @@ inline Environment::~Environment() { delete[] heap_statistics_buffer_; delete[] heap_space_statistics_buffer_; delete[] http_parser_buffer_; - free(http2_state_buffer_); + delete http2_state_; free(performance_state_); } @@ -445,7 +448,9 @@ inline std::vector* Environment::destroy_ids_list() { } inline double Environment::new_async_id() { - return ++async_hooks()->uid_fields()[AsyncHooks::kAsyncUidCntr]; + async_hooks()->uid_fields()[AsyncHooks::kAsyncUidCntr] = + async_hooks()->uid_fields()[AsyncHooks::kAsyncUidCntr] + 1; + return async_hooks()->uid_fields()[AsyncHooks::kAsyncUidCntr]; } inline double Environment::current_async_id() { @@ -457,7 +462,8 @@ inline double Environment::trigger_id() { } inline double Environment::get_init_trigger_id() { - double* uid_fields = async_hooks()->uid_fields(); + AliasedBuffer uid_fields = + async_hooks()->uid_fields(); double tid = uid_fields[AsyncHooks::kInitTriggerId]; uid_fields[AsyncHooks::kInitTriggerId] = 0; if (tid <= 0) tid = current_async_id(); @@ -497,13 +503,13 @@ inline void Environment::set_http_parser_buffer(char* buffer) { http_parser_buffer_ = buffer; } -inline http2::http2_state* Environment::http2_state_buffer() const { - return http2_state_buffer_; +inline http2::http2_state* Environment::http2_state() const { + return http2_state_; } -inline void Environment::set_http2_state_buffer(http2::http2_state* buffer) { - CHECK_EQ(http2_state_buffer_, nullptr); // Should be set only once. - http2_state_buffer_ = buffer; +inline void Environment::set_http2_state(http2::http2_state* buffer) { + CHECK_EQ(http2_state_, nullptr); // Should be set only once. + http2_state_ = buffer; } inline double* Environment::fs_stats_field_array() const { diff --git a/src/env.h b/src/env.h index fded721b804063..4d544ff6d13947 100644 --- a/src/env.h +++ b/src/env.h @@ -24,6 +24,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include "aliased_buffer.h" #include "ares.h" #if HAVE_INSPECTOR #include "inspector_agent.h" @@ -34,6 +35,7 @@ #include "uv.h" #include "v8.h" #include "node.h" +#include "node_http2_state.h" #include #include @@ -46,10 +48,6 @@ struct nghttp2_rcbuf; namespace node { -namespace http2 { -struct http2_state; -} - // Pick an index that's hopefully out of the way when we're embedded inside // another application. Performance-wise or memory-wise it doesn't matter: // Context::SetAlignedPointerInEmbedderData() is backed by a FixedArray, @@ -390,10 +388,12 @@ class Environment { AsyncHooks() = delete; - inline uint32_t* fields(); + inline AliasedBuffer& fields(); inline int fields_count() const; - inline double* uid_fields(); + + inline AliasedBuffer& uid_fields(); inline int uid_fields_count() const; + inline v8::Local provider_string(int idx); inline void push_ids(double async_id, double trigger_id); @@ -412,7 +412,7 @@ class Environment { private: Environment* env_; - double* uid_fields_ref_; + AliasedBuffer uid_fields_ref_; DISALLOW_COPY_AND_ASSIGN(InitScope); }; @@ -447,9 +447,9 @@ class Environment { std::stack ids_stack_; // Attached to a Uint32Array that tracks the number of active hooks for // each type. - uint32_t fields_[kFieldsCount]; + AliasedBuffer fields_; // Attached to a Float64Array that tracks the state of async resources. - double uid_fields_[kUidFieldsCount]; + AliasedBuffer uid_fields_; DISALLOW_COPY_AND_ASSIGN(AsyncHooks); }; @@ -608,8 +608,8 @@ class Environment { inline char* http_parser_buffer() const; inline void set_http_parser_buffer(char* buffer); - inline http2::http2_state* http2_state_buffer() const; - inline void set_http2_state_buffer(http2::http2_state* buffer); + inline http2::http2_state* http2_state() const; + inline void set_http2_state(http2::http2_state * state); inline double* fs_stats_field_array() const; inline void set_fs_stats_field_array(double* fields); @@ -736,7 +736,7 @@ class Environment { double* heap_space_statistics_buffer_ = nullptr; char* http_parser_buffer_; - http2::http2_state* http2_state_buffer_ = nullptr; + http2::http2_state* http2_state_ = nullptr; double* fs_stats_field_array_; diff --git a/src/node_http2.cc b/src/node_http2.cc index 610c8e8f2351ba..15c334771fdd2d 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1,10 +1,11 @@ +#include "aliased_buffer.h" #include "node.h" #include "node_buffer.h" #include "node_http2.h" +#include "node_http2_state.h" namespace node { -using v8::ArrayBuffer; using v8::Boolean; using v8::Context; using v8::Float64Array; @@ -17,64 +18,6 @@ using v8::Undefined; namespace http2 { -enum Http2SettingsIndex { - IDX_SETTINGS_HEADER_TABLE_SIZE, - IDX_SETTINGS_ENABLE_PUSH, - IDX_SETTINGS_INITIAL_WINDOW_SIZE, - IDX_SETTINGS_MAX_FRAME_SIZE, - IDX_SETTINGS_MAX_CONCURRENT_STREAMS, - IDX_SETTINGS_MAX_HEADER_LIST_SIZE, - IDX_SETTINGS_COUNT -}; - -enum Http2SessionStateIndex { - IDX_SESSION_STATE_EFFECTIVE_LOCAL_WINDOW_SIZE, - IDX_SESSION_STATE_EFFECTIVE_RECV_DATA_LENGTH, - IDX_SESSION_STATE_NEXT_STREAM_ID, - IDX_SESSION_STATE_LOCAL_WINDOW_SIZE, - IDX_SESSION_STATE_LAST_PROC_STREAM_ID, - IDX_SESSION_STATE_REMOTE_WINDOW_SIZE, - IDX_SESSION_STATE_OUTBOUND_QUEUE_SIZE, - IDX_SESSION_STATE_HD_DEFLATE_DYNAMIC_TABLE_SIZE, - IDX_SESSION_STATE_HD_INFLATE_DYNAMIC_TABLE_SIZE, - IDX_SESSION_STATE_COUNT -}; - -enum Http2StreamStateIndex { - IDX_STREAM_STATE, - IDX_STREAM_STATE_WEIGHT, - IDX_STREAM_STATE_SUM_DEPENDENCY_WEIGHT, - IDX_STREAM_STATE_LOCAL_CLOSE, - IDX_STREAM_STATE_REMOTE_CLOSE, - IDX_STREAM_STATE_LOCAL_WINDOW_SIZE, - IDX_STREAM_STATE_COUNT -}; - -enum Http2OptionsIndex { - IDX_OPTIONS_MAX_DEFLATE_DYNAMIC_TABLE_SIZE, - IDX_OPTIONS_MAX_RESERVED_REMOTE_STREAMS, - IDX_OPTIONS_MAX_SEND_HEADER_BLOCK_LENGTH, - IDX_OPTIONS_PEER_MAX_CONCURRENT_STREAMS, - IDX_OPTIONS_PADDING_STRATEGY, - IDX_OPTIONS_FLAGS -}; - -enum Http2PaddingBufferFields { - PADDING_BUF_FRAME_LENGTH, - PADDING_BUF_MAX_PAYLOAD_LENGTH, - PADDING_BUF_RETURN_VALUE, - PADDING_BUF_FIELD_COUNT -}; - -struct http2_state { - // doubles first so that they are always sizeof(double)-aligned - double session_state_buffer[IDX_SESSION_STATE_COUNT]; - double stream_state_buffer[IDX_STREAM_STATE_COUNT]; - uint32_t padding_buffer[PADDING_BUF_FIELD_COUNT]; - uint32_t options_buffer[IDX_OPTIONS_FLAGS + 1]; - uint32_t settings_buffer[IDX_SETTINGS_COUNT + 1]; -}; - Freelist data_chunk_free_list; @@ -92,7 +35,8 @@ Nghttp2Session::Callbacks Nghttp2Session::callback_struct_saved[2] = { Http2Options::Http2Options(Environment* env) { nghttp2_option_new(&options_); - uint32_t* buffer = env->http2_state_buffer()->options_buffer; + AliasedBuffer buffer = + env->http2_state()->options_buffer; uint32_t flags = buffer[IDX_OPTIONS_FLAGS]; if (flags & (1 << IDX_OPTIONS_MAX_DEFLATE_DYNAMIC_TABLE_SIZE)) { @@ -124,7 +68,7 @@ Http2Options::Http2Options(Environment* env) { if (flags & (1 << IDX_OPTIONS_PADDING_STRATEGY)) { padding_strategy_type strategy = static_cast( - buffer[IDX_OPTIONS_PADDING_STRATEGY]); + buffer.GetValue(IDX_OPTIONS_PADDING_STRATEGY)); SetPaddingStrategy(strategy); } } @@ -149,7 +93,8 @@ ssize_t Http2Session::OnCallbackPadding(size_t frameLen, Context::Scope context_scope(context); if (object()->Has(context, env()->ongetpadding_string()).FromJust()) { - uint32_t* buffer = env()->http2_state_buffer()->padding_buffer; + AliasedBuffer buffer = + env()->http2_state()->padding_buffer; buffer[PADDING_BUF_FRAME_LENGTH] = frameLen; buffer[PADDING_BUF_MAX_PAYLOAD_LENGTH] = maxPayloadLen; MakeCallback(env()->ongetpadding_string(), 0, nullptr); @@ -190,7 +135,8 @@ void PackSettings(const FunctionCallbackInfo& args) { std::vector entries; entries.reserve(6); - uint32_t* buffer = env->http2_state_buffer()->settings_buffer; + AliasedBuffer buffer = + env->http2_state()->settings_buffer; uint32_t flags = buffer[IDX_SETTINGS_COUNT]; if (flags & (1 << IDX_SETTINGS_HEADER_TABLE_SIZE)) { @@ -250,7 +196,9 @@ void PackSettings(const FunctionCallbackInfo& args) { void RefreshDefaultSettings(const FunctionCallbackInfo& args) { DEBUG_HTTP2("Http2Session: refreshing default settings\n"); Environment* env = Environment::GetCurrent(args); - uint32_t* buffer = env->http2_state_buffer()->settings_buffer; + AliasedBuffer buffer = + env->http2_state()->settings_buffer; + buffer[IDX_SETTINGS_HEADER_TABLE_SIZE] = DEFAULT_SETTINGS_HEADER_TABLE_SIZE; buffer[IDX_SETTINGS_ENABLE_PUSH] = @@ -276,7 +224,8 @@ void RefreshSettings(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&session, args[0].As()); nghttp2_session* s = session->session(); - uint32_t* buffer = env->http2_state_buffer()->settings_buffer; + AliasedBuffer buffer = + env->http2_state()->settings_buffer; buffer[IDX_SETTINGS_HEADER_TABLE_SIZE] = fn(s, NGHTTP2_SETTINGS_HEADER_TABLE_SIZE); buffer[IDX_SETTINGS_MAX_CONCURRENT_STREAMS] = @@ -297,7 +246,8 @@ void RefreshSessionState(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); CHECK_EQ(args.Length(), 1); CHECK(args[0]->IsObject()); - double* buffer = env->http2_state_buffer()->session_state_buffer; + AliasedBuffer buffer = + env->http2_state()->session_state_buffer; Http2Session* session; ASSIGN_OR_RETURN_UNWRAP(&session, args[0].As()); nghttp2_session* s = session->session(); @@ -334,7 +284,8 @@ void RefreshStreamState(const FunctionCallbackInfo& args) { nghttp2_session* s = session->session(); Nghttp2Stream* stream; - double* buffer = env->http2_state_buffer()->stream_state_buffer; + AliasedBuffer buffer = + env->http2_state()->stream_state_buffer; if ((stream = session->FindStream(id)) == nullptr) { buffer[IDX_STREAM_STATE] = NGHTTP2_STREAM_STATE_IDLE; @@ -446,7 +397,8 @@ void Http2Session::SubmitSettings(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); Environment* env = session->env(); - uint32_t* buffer = env->http2_state_buffer()->settings_buffer; + AliasedBuffer buffer = + env->http2_state()->settings_buffer; uint32_t flags = buffer[IDX_SETTINGS_COUNT]; std::vector entries; @@ -1177,26 +1129,27 @@ void Initialize(Local target, Isolate* isolate = env->isolate(); HandleScope scope(isolate); - http2_state* state = Calloc(1); - env->set_http2_state_buffer(state); - auto state_ab = ArrayBuffer::New(isolate, state, sizeof(*state)); + http2_state* state = new http2_state(isolate); + env->set_http2_state(state); -#define SET_STATE_TYPEDARRAY(name, type, field) \ - target->Set(context, \ - FIXED_ONE_BYTE_STRING(isolate, (name)), \ - type::New(state_ab, \ - offsetof(http2_state, field), \ - arraysize(state->field))) \ - .FromJust() +#define SET_STATE_TYPEDARRAY(name, field) \ + target->Set(context, \ + FIXED_ONE_BYTE_STRING(isolate, (name)), \ + (field)).FromJust() // Initialize the buffer used for padding callbacks - SET_STATE_TYPEDARRAY("paddingBuffer", Uint32Array, padding_buffer); + SET_STATE_TYPEDARRAY( + "paddingBuffer", state->padding_buffer.GetJSArray()); // Initialize the buffer used to store the session state - SET_STATE_TYPEDARRAY("sessionState", Float64Array, session_state_buffer); + SET_STATE_TYPEDARRAY( + "sessionState", state->session_state_buffer.GetJSArray()); // Initialize the buffer used to store the stream state - SET_STATE_TYPEDARRAY("streamState", Float64Array, stream_state_buffer); - SET_STATE_TYPEDARRAY("settingsBuffer", Uint32Array, settings_buffer); - SET_STATE_TYPEDARRAY("optionsBuffer", Uint32Array, options_buffer); + SET_STATE_TYPEDARRAY( + "streamState", state->stream_state_buffer.GetJSArray()); + SET_STATE_TYPEDARRAY( + "settingsBuffer", state->settings_buffer.GetJSArray()); + SET_STATE_TYPEDARRAY( + "optionsBuffer", state->options_buffer.GetJSArray()); #undef SET_STATE_TYPEDARRAY NODE_DEFINE_CONSTANT(target, PADDING_BUF_FRAME_LENGTH); diff --git a/src/node_http2.h b/src/node_http2.h index 107e87a2dc0f69..a5029bec378765 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -4,6 +4,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include "node_http2_core-inl.h" +#include "node_http2_state.h" #include "stream_base-inl.h" #include "string_bytes.h" diff --git a/src/node_http2_state.h b/src/node_http2_state.h new file mode 100755 index 00000000000000..a945f07b686b4a --- /dev/null +++ b/src/node_http2_state.h @@ -0,0 +1,116 @@ +#ifndef SRC_NODE_HTTP2_STATE_H_ +#define SRC_NODE_HTTP2_STATE_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#include "aliased_buffer.h" + +namespace node { +namespace http2 { + + enum Http2SettingsIndex { + IDX_SETTINGS_HEADER_TABLE_SIZE, + IDX_SETTINGS_ENABLE_PUSH, + IDX_SETTINGS_INITIAL_WINDOW_SIZE, + IDX_SETTINGS_MAX_FRAME_SIZE, + IDX_SETTINGS_MAX_CONCURRENT_STREAMS, + IDX_SETTINGS_MAX_HEADER_LIST_SIZE, + IDX_SETTINGS_COUNT + }; + + enum Http2SessionStateIndex { + IDX_SESSION_STATE_EFFECTIVE_LOCAL_WINDOW_SIZE, + IDX_SESSION_STATE_EFFECTIVE_RECV_DATA_LENGTH, + IDX_SESSION_STATE_NEXT_STREAM_ID, + IDX_SESSION_STATE_LOCAL_WINDOW_SIZE, + IDX_SESSION_STATE_LAST_PROC_STREAM_ID, + IDX_SESSION_STATE_REMOTE_WINDOW_SIZE, + IDX_SESSION_STATE_OUTBOUND_QUEUE_SIZE, + IDX_SESSION_STATE_HD_DEFLATE_DYNAMIC_TABLE_SIZE, + IDX_SESSION_STATE_HD_INFLATE_DYNAMIC_TABLE_SIZE, + IDX_SESSION_STATE_COUNT + }; + + enum Http2StreamStateIndex { + IDX_STREAM_STATE, + IDX_STREAM_STATE_WEIGHT, + IDX_STREAM_STATE_SUM_DEPENDENCY_WEIGHT, + IDX_STREAM_STATE_LOCAL_CLOSE, + IDX_STREAM_STATE_REMOTE_CLOSE, + IDX_STREAM_STATE_LOCAL_WINDOW_SIZE, + IDX_STREAM_STATE_COUNT + }; + + enum Http2OptionsIndex { + IDX_OPTIONS_MAX_DEFLATE_DYNAMIC_TABLE_SIZE, + IDX_OPTIONS_MAX_RESERVED_REMOTE_STREAMS, + IDX_OPTIONS_MAX_SEND_HEADER_BLOCK_LENGTH, + IDX_OPTIONS_PEER_MAX_CONCURRENT_STREAMS, + IDX_OPTIONS_PADDING_STRATEGY, + IDX_OPTIONS_FLAGS + }; + + enum Http2PaddingBufferFields { + PADDING_BUF_FRAME_LENGTH, + PADDING_BUF_MAX_PAYLOAD_LENGTH, + PADDING_BUF_RETURN_VALUE, + PADDING_BUF_FIELD_COUNT + }; + +class http2_state { + public: + explicit http2_state(v8::Isolate* isolate) : + root_buffer( + isolate, + sizeof(http2_state_internal)), + session_state_buffer( + isolate, + offsetof(http2_state_internal, session_state_buffer), + IDX_SESSION_STATE_COUNT, + root_buffer), + stream_state_buffer( + isolate, + offsetof(http2_state_internal, stream_state_buffer), + IDX_STREAM_STATE_COUNT, + root_buffer), + padding_buffer( + isolate, + offsetof(http2_state_internal, padding_buffer), + PADDING_BUF_FIELD_COUNT, + root_buffer), + options_buffer( + isolate, + offsetof(http2_state_internal, options_buffer), + IDX_OPTIONS_FLAGS + 1, + root_buffer), + settings_buffer( + isolate, + offsetof(http2_state_internal, settings_buffer), + IDX_SETTINGS_COUNT + 1, + root_buffer) { + } + + AliasedBuffer root_buffer; + AliasedBuffer session_state_buffer; + AliasedBuffer stream_state_buffer; + AliasedBuffer padding_buffer; + AliasedBuffer options_buffer; + AliasedBuffer settings_buffer; + + private: + struct http2_state_internal { + // doubles first so that they are always sizeof(double)-aligned + double session_state_buffer[IDX_SESSION_STATE_COUNT]; + double stream_state_buffer[IDX_STREAM_STATE_COUNT]; + uint32_t padding_buffer[PADDING_BUF_FIELD_COUNT]; + uint32_t options_buffer[IDX_OPTIONS_FLAGS + 1]; + uint32_t settings_buffer[IDX_SETTINGS_COUNT + 1]; + }; +}; + +} // namespace http2 +} // namespace node + +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#endif // SRC_NODE_HTTP2_STATE_H_ diff --git a/test/cctest/node_test_fixture.cc b/test/cctest/node_test_fixture.cc new file mode 100644 index 00000000000000..477adb5711af38 --- /dev/null +++ b/test/cctest/node_test_fixture.cc @@ -0,0 +1,3 @@ +#include "node_test_fixture.h" + +uv_loop_t current_loop; diff --git a/test/cctest/node_test_fixture.h b/test/cctest/node_test_fixture.h index f30823a8fdb46a..79027d25ad8c8d 100644 --- a/test/cctest/node_test_fixture.h +++ b/test/cctest/node_test_fixture.h @@ -66,7 +66,7 @@ struct Argv { int nr_args_; }; -uv_loop_t current_loop; +extern uv_loop_t current_loop; class NodeTestFixture : public ::testing::Test { public: diff --git a/test/cctest/test_aliased_buffer.cc b/test/cctest/test_aliased_buffer.cc new file mode 100644 index 00000000000000..251b0c7eb7a83c --- /dev/null +++ b/test/cctest/test_aliased_buffer.cc @@ -0,0 +1,220 @@ + +#include "v8.h" +#include "aliased_buffer.h" +#include "node_test_fixture.h" + +using node::AliasedBuffer; + +class AliasBufferTest : public NodeTestFixture { + protected: + void SetUp() override { + NodeTestFixture::SetUp(); + } + + void TearDown() override { + NodeTestFixture::TearDown(); + } +}; + +template +void CreateOracleValues(NativeT* buf, size_t count) { + for (size_t i = 0, j = count; i < count; i++, j--) { + buf[i] = static_cast(j); + } +} + +template +void WriteViaOperator( + AliasedBuffer& aliasedBuffer, // NOLINT(runtime/references) + size_t size, + NativeT* oracle) { + // write through the API + for (size_t i = 0; i < size; i++) { + aliasedBuffer[i] = oracle[i]; + } +} + +template +void WriteViaSetValue( + AliasedBuffer& aliasedBuffer, // NOLINT(runtime/references) + size_t size, NativeT* oracle) { + // write through the API + for (size_t i = 0; i < size; i++) { + aliasedBuffer.SetValue(i, oracle[i]); + } +} + +template +void ReadAndValidate( + v8::Isolate* isolate, + v8::Local context, + AliasedBuffer& aliasedBuffer, // NOLINT(runtime/references) + size_t size, + NativeT* oracle) { + // read through the API + for (size_t i = 0; i < size; i++) { + NativeT v1 = (NativeT)aliasedBuffer[i]; + NativeT v2 = (NativeT)aliasedBuffer.GetValue(i); + EXPECT_TRUE(v1 == oracle[i]); + EXPECT_TRUE(v2 == oracle[i]); + } + + // validate size of JS Buffer + EXPECT_TRUE(aliasedBuffer.GetJSArray()->Length() == size); + EXPECT_TRUE( + aliasedBuffer.GetJSArray()->ByteLength() == + (size * sizeof(NativeT))); + + // validate operator * and GetBuffer are the same + EXPECT_TRUE(aliasedBuffer.GetNativeBuffer() == *aliasedBuffer); + + // read through the JS API + for (size_t i = 0; i < size; i++) { + v8::Local v8TypedArray = aliasedBuffer.GetJSArray(); + v8::MaybeLocal v = v8TypedArray->Get(context, i); + EXPECT_TRUE(v.IsEmpty() == false); + v8::Local v2 = v.ToLocalChecked(); + EXPECT_TRUE(v2->IsNumber()); + // v8::MaybeLocal v3 = v2->ToNumber(); + // v8::Local v4 = v3.ToLocalChecked(); + // NativeT actualValue = *(reinterpret_cast(&v4->Value())); + // EXPECT_TRUE((NativeT)(v4->Value()) == oracle[i]); + // TODO(@mike-kaufman) figure out how to compare the value in the v8 array + // Expect_TRUE(*v.ToLocal() == v8NumberCreateFunction(oracle[i]); + } +} + +template +void ReadWriteTest(v8::Isolate* isolate) { + v8::HandleScope handle_scope(isolate); + v8::Local context = v8::Context::New(isolate); + v8::Context::Scope context_scope(context); + + const size_t size = 100; + AliasedBuffer ab(isolate, size); + NativeT* oracle = new NativeT[size]; + CreateOracleValues(oracle, size); + WriteViaOperator(ab, size, oracle); + ReadAndValidate(isolate, context, ab, size, oracle); + + WriteViaSetValue(ab, size, oracle); + + // validate copy constructor + { + AliasedBuffer ab2(ab); + ReadAndValidate(isolate, context, ab2, size, oracle); + } + ReadAndValidate(isolate, context, ab, size, oracle); + + delete[] oracle; +} + +template< + class NativeT_A, class V8T_A, + class NativeT_B, class V8T_B, + class NativeT_C, class V8T_C> +void SharedBufferTest( + v8::Isolate* isolate, + size_t count_A, + size_t count_B, + size_t count_C) { + v8::HandleScope handle_scope(isolate); + v8::Local context = v8::Context::New(isolate); + v8::Context::Scope context_scope(context); + + size_t sizeInBytes_A = count_A * sizeof(NativeT_A); + size_t sizeInBytes_B = count_B * sizeof(NativeT_B); + size_t sizeInBytes_C = count_C * sizeof(NativeT_C); + + AliasedBuffer rootBuffer( + isolate, sizeInBytes_A + sizeInBytes_B + sizeInBytes_C); + AliasedBuffer ab_A( + isolate, 0, count_A, rootBuffer); + AliasedBuffer ab_B( + isolate, sizeInBytes_A, count_B, rootBuffer); + AliasedBuffer ab_C( + isolate, sizeInBytes_A + sizeInBytes_B, count_C, rootBuffer); + + NativeT_A* oracle_A = new NativeT_A[count_A]; + NativeT_B* oracle_B = new NativeT_B[count_B]; + NativeT_C* oracle_C = new NativeT_C[count_C]; + CreateOracleValues(oracle_A, count_A); + CreateOracleValues(oracle_B, count_B); + CreateOracleValues(oracle_C, count_C); + + WriteViaOperator(ab_A, count_A, oracle_A); + WriteViaOperator(ab_B, count_B, oracle_B); + WriteViaOperator(ab_C, count_C, oracle_C); + + ReadAndValidate(isolate, context, ab_A, count_A, oracle_A); + ReadAndValidate(isolate, context, ab_B, count_B, oracle_B); + ReadAndValidate(isolate, context, ab_C, count_C, oracle_C); + + WriteViaSetValue(ab_A, count_A, oracle_A); + WriteViaSetValue(ab_B, count_B, oracle_B); + WriteViaSetValue(ab_C, count_C, oracle_C); + + ReadAndValidate(isolate, context, ab_A, count_A, oracle_A); + ReadAndValidate(isolate, context, ab_B, count_B, oracle_B); + ReadAndValidate(isolate, context, ab_C, count_C, oracle_C); +} + +TEST_F(AliasBufferTest, Uint8Array) { + ReadWriteTest(isolate_); +} + +TEST_F(AliasBufferTest, Int8Array) { + ReadWriteTest(isolate_); +} + +TEST_F(AliasBufferTest, Uint16Array) { + ReadWriteTest(isolate_); +} + +TEST_F(AliasBufferTest, Int16Array) { + ReadWriteTest(isolate_); +} + +TEST_F(AliasBufferTest, Uint32Array) { + ReadWriteTest(isolate_); +} + +TEST_F(AliasBufferTest, Int32Array) { + ReadWriteTest(isolate_); +} + +TEST_F(AliasBufferTest, Float32Array) { + ReadWriteTest(isolate_); +} + +TEST_F(AliasBufferTest, Float64Array) { + ReadWriteTest(isolate_); +} + +TEST_F(AliasBufferTest, SharedArrayBuffer1) { + SharedBufferTest< + uint32_t, v8::Uint32Array, + double, v8::Float64Array, + int8_t, v8::Int8Array>(isolate_, 100, 80, 8); +} + +TEST_F(AliasBufferTest, SharedArrayBuffer2) { + SharedBufferTest< + double, v8::Float64Array, + int8_t, v8::Int8Array, + double, v8::Float64Array>(isolate_, 100, 8, 8); +} + +TEST_F(AliasBufferTest, SharedArrayBuffer3) { + SharedBufferTest< + int8_t, v8::Int8Array, + int8_t, v8::Int8Array, + double, v8::Float64Array>(isolate_, 1, 7, 8); +} + +TEST_F(AliasBufferTest, SharedArrayBuffer4) { + SharedBufferTest< + int8_t, v8::Int8Array, + int8_t, v8::Int8Array, + int32_t, v8::Int32Array>(isolate_, 1, 3, 1); +} From a47238b091a476b86e22c747db9cac704afde331 Mon Sep 17 00:00:00 2001 From: Mike Kaufman Date: Thu, 31 Aug 2017 10:47:43 -0700 Subject: [PATCH 2/4] responding to PR feedback: cleaning up whitespace, snake-casing member/variable names, adding const, some other stuff... --- src/aliased_buffer.h | 107 ++++++++++++++--------------- src/env-inl.h | 4 +- src/node_http2.cc | 32 ++++----- test/cctest/test_aliased_buffer.cc | 80 +++++++++++---------- 4 files changed, 109 insertions(+), 114 deletions(-) diff --git a/src/aliased_buffer.h b/src/aliased_buffer.h index 480c3a9f42815f..fcbe42447d546f 100644 --- a/src/aliased_buffer.h +++ b/src/aliased_buffer.h @@ -26,27 +26,24 @@ template class AliasedBuffer { public: AliasedBuffer(v8::Isolate* isolate, const size_t count) : - isolate_(isolate), - count_(count), - byte_offset_(0), - freeBuffer_(true) { - const v8::HandleScope handle_scope(this->isolate_); + isolate_(isolate), + count_(count), + byte_offset_(0), + free_buffer_(true) { + const v8::HandleScope handle_scope(isolate_); const size_t sizeInBytes = sizeof(NativeT) * count; // allocate native buffer - this->buffer_ = UncheckedCalloc(count); - if (this->buffer_ == nullptr) { - ABORT(); - } + buffer_ = Calloc(count); // allocate v8 ArrayBuffer v8::Local ab = v8::ArrayBuffer::New( - this->isolate_, this->buffer_, sizeInBytes); + isolate_, buffer_, sizeInBytes); // allocate v8 TypedArray - v8::Local jsArray = V8T::New(ab, this->byte_offset_, count); - this->jsArray_ = v8::Global(isolate, jsArray); + v8::Local js_array = V8T::New(ab, byte_offset_, count); + js_array_ = v8::Global(isolate, js_array); } /** @@ -58,40 +55,40 @@ class AliasedBuffer { * Note that byte_offset must by aligned by sizeof(NativeT). */ AliasedBuffer( - v8::Isolate* isolate, - const size_t byte_offset, - const size_t count, - const AliasedBuffer& backingBuffer) : - isolate_(isolate), - count_(count), - byte_offset_(byte_offset), - freeBuffer_(false) { - const v8::HandleScope handle_scope(this->isolate_); + v8::Isolate* isolate, + const size_t byte_offset, + const size_t count, + const AliasedBuffer& backing_buffer) : + isolate_(isolate), + count_(count), + byte_offset_(byte_offset), + free_buffer_(false) { + const v8::HandleScope handle_scope(isolate_); // validate that the byte_offset is aligned with sizeof(NativeT) CHECK_EQ(byte_offset & (sizeof(NativeT) - 1), 0); - this->buffer_ = reinterpret_cast( - const_cast(backingBuffer.GetNativeBuffer() + byte_offset)); + buffer_ = reinterpret_cast( + const_cast(backing_buffer.GetNativeBuffer() + byte_offset)); - v8::Local ab = backingBuffer.GetArrayBuffer(); - v8::Local jsArray = V8T::New(ab, byte_offset, count); - this->jsArray_ = v8::Global(isolate, jsArray); + v8::Local ab = backing_buffer.GetArrayBuffer(); + v8::Local js_array = V8T::New(ab, byte_offset, count); + js_array_ = v8::Global(isolate, js_array); } AliasedBuffer(const AliasedBuffer& that) : - isolate_(that.isolate_), - count_(that.count_), - byte_offset_(that.byte_offset_), - buffer_(that.buffer_), - freeBuffer_(false) { - this->jsArray_ = v8::Global(that.isolate_, that.GetJSArray()); + isolate_(that.isolate_), + count_(that.count_), + byte_offset_(that.byte_offset_), + buffer_(that.buffer_), + free_buffer_(false) { + js_array_ = v8::Global(that.isolate_, that.GetJSArray()); } ~AliasedBuffer() { - if (this->freeBuffer_ && this->buffer_ != NULL) { - free(this->buffer_); + if (free_buffer_ && buffer_ != NULL) { + free(buffer_); } - this->jsArray_.Reset(); + js_array_.Reset(); } /** @@ -100,27 +97,27 @@ class AliasedBuffer { */ class Reference { public: - Reference(AliasedBuffer* aliasedBuffer, size_t index) : - aliasedBuffer_(aliasedBuffer), + Reference(AliasedBuffer* aliased_buffer, size_t index) : + aliased_buffer_(aliased_buffer), index_(index) { } Reference(const Reference& that) : - aliasedBuffer_(that.aliasedBuffer_), + aliased_buffer_(that.aliased_buffer_), index_(that.index_) { } inline Reference& operator=(const NativeT &val) { - this->aliasedBuffer_->SetValue(this->index_, val); + aliased_buffer_->SetValue(index_, val); return *this; } - operator NativeT() { - return this->aliasedBuffer_->GetValue(this->index_); + operator NativeT() const { + return aliased_buffer_->GetValue(index_); } private: - AliasedBuffer* aliasedBuffer_; + AliasedBuffer* aliased_buffer_; size_t index_; }; @@ -128,7 +125,7 @@ class AliasedBuffer { * Get the underlying v8 TypedArray overlayed on top of the native buffer */ v8::Local GetJSArray() const { - return this->jsArray_.Get(this->isolate_); + return js_array_.Get(isolate_); } /** @@ -136,7 +133,7 @@ class AliasedBuffer { * overlaying the native buffer */ v8::Local GetArrayBuffer() const { - return this->GetJSArray()->Buffer(); + return GetJSArray()->Buffer(); } /** @@ -144,30 +141,30 @@ class AliasedBuffer { * through the GetValue/SetValue/operator[] methods */ inline const NativeT* GetNativeBuffer() const { - return this->buffer_; + return buffer_; } /** * Synonym for GetBuffer() */ - inline const NativeT* operator * () { - return this->GetNativeBuffer(); + inline const NativeT* operator * () const { + return GetNativeBuffer(); } /** * Set position index to given value. */ inline void SetValue(const size_t index, NativeT value) { - CHECK_LT(index, this->count_); - this->buffer_[index] = value; + CHECK_LT(index, count_); + buffer_[index] = value; } /** * Get value at position index */ inline const NativeT GetValue(const size_t index) const { - CHECK_LT(index, this->count_); - return this->buffer_[index]; + CHECK_LT(index, count_); + return buffer_[index]; } /** @@ -178,7 +175,7 @@ class AliasedBuffer { } NativeT operator[](size_t index) const { - return this->GetValue(index); + return GetValue(index); } private: @@ -186,8 +183,8 @@ class AliasedBuffer { size_t count_; size_t byte_offset_; NativeT* buffer_; - v8::Global jsArray_; - bool freeBuffer_; + v8::Global js_array_; + bool free_buffer_; }; } // namespace node diff --git a/src/env-inl.h b/src/env-inl.h index 6a2fca1284ae39..555e1f6f5a8957 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -349,7 +349,7 @@ inline Environment::~Environment() { delete[] heap_statistics_buffer_; delete[] heap_space_statistics_buffer_; delete[] http_parser_buffer_; - delete http2_state_; + delete http2_state_; free(performance_state_); } @@ -462,7 +462,7 @@ inline double Environment::trigger_id() { } inline double Environment::get_init_trigger_id() { - AliasedBuffer uid_fields = + AliasedBuffer& uid_fields = async_hooks()->uid_fields(); double tid = uid_fields[AsyncHooks::kInitTriggerId]; uid_fields[AsyncHooks::kInitTriggerId] = 0; diff --git a/src/node_http2.cc b/src/node_http2.cc index 15c334771fdd2d..65528fc98eed13 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -35,8 +35,8 @@ Nghttp2Session::Callbacks Nghttp2Session::callback_struct_saved[2] = { Http2Options::Http2Options(Environment* env) { nghttp2_option_new(&options_); - AliasedBuffer buffer = - env->http2_state()->options_buffer; + AliasedBuffer& buffer = + env->http2_state()->options_buffer; uint32_t flags = buffer[IDX_OPTIONS_FLAGS]; if (flags & (1 << IDX_OPTIONS_MAX_DEFLATE_DYNAMIC_TABLE_SIZE)) { @@ -93,8 +93,8 @@ ssize_t Http2Session::OnCallbackPadding(size_t frameLen, Context::Scope context_scope(context); if (object()->Has(context, env()->ongetpadding_string()).FromJust()) { - AliasedBuffer buffer = - env()->http2_state()->padding_buffer; + AliasedBuffer& buffer = + env()->http2_state()->padding_buffer; buffer[PADDING_BUF_FRAME_LENGTH] = frameLen; buffer[PADDING_BUF_MAX_PAYLOAD_LENGTH] = maxPayloadLen; MakeCallback(env()->ongetpadding_string(), 0, nullptr); @@ -135,8 +135,8 @@ void PackSettings(const FunctionCallbackInfo& args) { std::vector entries; entries.reserve(6); - AliasedBuffer buffer = - env->http2_state()->settings_buffer; + AliasedBuffer& buffer = + env->http2_state()->settings_buffer; uint32_t flags = buffer[IDX_SETTINGS_COUNT]; if (flags & (1 << IDX_SETTINGS_HEADER_TABLE_SIZE)) { @@ -196,8 +196,8 @@ void PackSettings(const FunctionCallbackInfo& args) { void RefreshDefaultSettings(const FunctionCallbackInfo& args) { DEBUG_HTTP2("Http2Session: refreshing default settings\n"); Environment* env = Environment::GetCurrent(args); - AliasedBuffer buffer = - env->http2_state()->settings_buffer; + AliasedBuffer& buffer = + env->http2_state()->settings_buffer; buffer[IDX_SETTINGS_HEADER_TABLE_SIZE] = DEFAULT_SETTINGS_HEADER_TABLE_SIZE; @@ -224,8 +224,8 @@ void RefreshSettings(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&session, args[0].As()); nghttp2_session* s = session->session(); - AliasedBuffer buffer = - env->http2_state()->settings_buffer; + AliasedBuffer& buffer = + env->http2_state()->settings_buffer; buffer[IDX_SETTINGS_HEADER_TABLE_SIZE] = fn(s, NGHTTP2_SETTINGS_HEADER_TABLE_SIZE); buffer[IDX_SETTINGS_MAX_CONCURRENT_STREAMS] = @@ -246,8 +246,8 @@ void RefreshSessionState(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); CHECK_EQ(args.Length(), 1); CHECK(args[0]->IsObject()); - AliasedBuffer buffer = - env->http2_state()->session_state_buffer; + AliasedBuffer& buffer = + env->http2_state()->session_state_buffer; Http2Session* session; ASSIGN_OR_RETURN_UNWRAP(&session, args[0].As()); nghttp2_session* s = session->session(); @@ -284,8 +284,8 @@ void RefreshStreamState(const FunctionCallbackInfo& args) { nghttp2_session* s = session->session(); Nghttp2Stream* stream; - AliasedBuffer buffer = - env->http2_state()->stream_state_buffer; + AliasedBuffer& buffer = + env->http2_state()->stream_state_buffer; if ((stream = session->FindStream(id)) == nullptr) { buffer[IDX_STREAM_STATE] = NGHTTP2_STREAM_STATE_IDLE; @@ -397,8 +397,8 @@ void Http2Session::SubmitSettings(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); Environment* env = session->env(); - AliasedBuffer buffer = - env->http2_state()->settings_buffer; + AliasedBuffer& buffer = + env->http2_state()->settings_buffer; uint32_t flags = buffer[IDX_SETTINGS_COUNT]; std::vector entries; diff --git a/test/cctest/test_aliased_buffer.cc b/test/cctest/test_aliased_buffer.cc index 251b0c7eb7a83c..500b4da485fb7e 100644 --- a/test/cctest/test_aliased_buffer.cc +++ b/test/cctest/test_aliased_buffer.cc @@ -25,9 +25,9 @@ void CreateOracleValues(NativeT* buf, size_t count) { template void WriteViaOperator( - AliasedBuffer& aliasedBuffer, // NOLINT(runtime/references) - size_t size, - NativeT* oracle) { + AliasedBuffer& aliasedBuffer, // NOLINT(runtime/references) + size_t size, + NativeT* oracle) { // write through the API for (size_t i = 0; i < size; i++) { aliasedBuffer[i] = oracle[i]; @@ -36,8 +36,8 @@ void WriteViaOperator( template void WriteViaSetValue( - AliasedBuffer& aliasedBuffer, // NOLINT(runtime/references) - size_t size, NativeT* oracle) { + AliasedBuffer& aliasedBuffer, // NOLINT(runtime/references) + size_t size, NativeT* oracle) { // write through the API for (size_t i = 0; i < size; i++) { aliasedBuffer.SetValue(i, oracle[i]); @@ -46,15 +46,15 @@ void WriteViaSetValue( template void ReadAndValidate( - v8::Isolate* isolate, - v8::Local context, - AliasedBuffer& aliasedBuffer, // NOLINT(runtime/references) - size_t size, - NativeT* oracle) { + v8::Isolate* isolate, + v8::Local context, + AliasedBuffer& aliasedBuffer, // NOLINT(runtime/references) + size_t size, + NativeT* oracle) { // read through the API for (size_t i = 0; i < size; i++) { - NativeT v1 = (NativeT)aliasedBuffer[i]; - NativeT v2 = (NativeT)aliasedBuffer.GetValue(i); + NativeT v1 = aliasedBuffer[i]; + NativeT v2 = aliasedBuffer.GetValue(i); EXPECT_TRUE(v1 == oracle[i]); EXPECT_TRUE(v2 == oracle[i]); } @@ -75,12 +75,10 @@ void ReadAndValidate( EXPECT_TRUE(v.IsEmpty() == false); v8::Local v2 = v.ToLocalChecked(); EXPECT_TRUE(v2->IsNumber()); - // v8::MaybeLocal v3 = v2->ToNumber(); - // v8::Local v4 = v3.ToLocalChecked(); - // NativeT actualValue = *(reinterpret_cast(&v4->Value())); - // EXPECT_TRUE((NativeT)(v4->Value()) == oracle[i]); - // TODO(@mike-kaufman) figure out how to compare the value in the v8 array - // Expect_TRUE(*v.ToLocal() == v8NumberCreateFunction(oracle[i]); + v8::MaybeLocal v3 = v2->ToNumber(context); + v8::Local v4 = v3.ToLocalChecked(); + NativeT actualValue = static_cast(v4->Value()); + EXPECT_TRUE(actualValue == oracle[i]); } } @@ -110,14 +108,14 @@ void ReadWriteTest(v8::Isolate* isolate) { } template< - class NativeT_A, class V8T_A, - class NativeT_B, class V8T_B, - class NativeT_C, class V8T_C> + class NativeT_A, class V8T_A, + class NativeT_B, class V8T_B, + class NativeT_C, class V8T_C> void SharedBufferTest( - v8::Isolate* isolate, - size_t count_A, - size_t count_B, - size_t count_C) { + v8::Isolate* isolate, + size_t count_A, + size_t count_B, + size_t count_C) { v8::HandleScope handle_scope(isolate); v8::Local context = v8::Context::New(isolate); v8::Context::Scope context_scope(context); @@ -127,13 +125,13 @@ void SharedBufferTest( size_t sizeInBytes_C = count_C * sizeof(NativeT_C); AliasedBuffer rootBuffer( - isolate, sizeInBytes_A + sizeInBytes_B + sizeInBytes_C); + isolate, sizeInBytes_A + sizeInBytes_B + sizeInBytes_C); AliasedBuffer ab_A( - isolate, 0, count_A, rootBuffer); + isolate, 0, count_A, rootBuffer); AliasedBuffer ab_B( - isolate, sizeInBytes_A, count_B, rootBuffer); + isolate, sizeInBytes_A, count_B, rootBuffer); AliasedBuffer ab_C( - isolate, sizeInBytes_A + sizeInBytes_B, count_C, rootBuffer); + isolate, sizeInBytes_A + sizeInBytes_B, count_C, rootBuffer); NativeT_A* oracle_A = new NativeT_A[count_A]; NativeT_B* oracle_B = new NativeT_B[count_B]; @@ -193,28 +191,28 @@ TEST_F(AliasBufferTest, Float64Array) { TEST_F(AliasBufferTest, SharedArrayBuffer1) { SharedBufferTest< - uint32_t, v8::Uint32Array, - double, v8::Float64Array, - int8_t, v8::Int8Array>(isolate_, 100, 80, 8); + uint32_t, v8::Uint32Array, + double, v8::Float64Array, + int8_t, v8::Int8Array>(isolate_, 100, 80, 8); } TEST_F(AliasBufferTest, SharedArrayBuffer2) { SharedBufferTest< - double, v8::Float64Array, - int8_t, v8::Int8Array, - double, v8::Float64Array>(isolate_, 100, 8, 8); + double, v8::Float64Array, + int8_t, v8::Int8Array, + double, v8::Float64Array>(isolate_, 100, 8, 8); } TEST_F(AliasBufferTest, SharedArrayBuffer3) { SharedBufferTest< - int8_t, v8::Int8Array, - int8_t, v8::Int8Array, - double, v8::Float64Array>(isolate_, 1, 7, 8); + int8_t, v8::Int8Array, + int8_t, v8::Int8Array, + double, v8::Float64Array>(isolate_, 1, 7, 8); } TEST_F(AliasBufferTest, SharedArrayBuffer4) { SharedBufferTest< - int8_t, v8::Int8Array, - int8_t, v8::Int8Array, - int32_t, v8::Int32Array>(isolate_, 1, 3, 1); + int8_t, v8::Int8Array, + int8_t, v8::Int8Array, + int32_t, v8::Int32Array>(isolate_, 1, 3, 1); } From d06a833888104553de06cf7308539a210e4ce982 Mon Sep 17 00:00:00 2001 From: Mike Kaufman Date: Sun, 10 Sep 2017 12:00:59 -0700 Subject: [PATCH 3/4] responding to PR feedback and fixing up whitespace/formatting --- src/aliased_buffer.h | 68 +++++++++++++++------------ test/cctest/test_aliased_buffer.cc | 74 +++++++++++++++--------------- 2 files changed, 74 insertions(+), 68 deletions(-) diff --git a/src/aliased_buffer.h b/src/aliased_buffer.h index fcbe42447d546f..1a6ebe2e3fd5ae 100644 --- a/src/aliased_buffer.h +++ b/src/aliased_buffer.h @@ -25,11 +25,12 @@ namespace node { template class AliasedBuffer { public: - AliasedBuffer(v8::Isolate* isolate, const size_t count) : - isolate_(isolate), - count_(count), - byte_offset_(0), - free_buffer_(true) { + AliasedBuffer(v8::Isolate* isolate, const size_t count) + : isolate_(isolate), + count_(count), + byte_offset_(0), + free_buffer_(true) { + CHECK_GT(count, 0); const v8::HandleScope handle_scope(isolate_); const size_t sizeInBytes = sizeof(NativeT) * count; @@ -54,19 +55,22 @@ class AliasedBuffer { * * Note that byte_offset must by aligned by sizeof(NativeT). */ - AliasedBuffer( - v8::Isolate* isolate, - const size_t byte_offset, - const size_t count, - const AliasedBuffer& backing_buffer) : - isolate_(isolate), - count_(count), - byte_offset_(byte_offset), - free_buffer_(false) { + AliasedBuffer(v8::Isolate* isolate, + const size_t byte_offset, + const size_t count, + const AliasedBuffer& backing_buffer) + : isolate_(isolate), + count_(count), + byte_offset_(byte_offset), + free_buffer_(false) { const v8::HandleScope handle_scope(isolate_); // validate that the byte_offset is aligned with sizeof(NativeT) CHECK_EQ(byte_offset & (sizeof(NativeT) - 1), 0); + // validate this fits inside the backing buffer + CHECK_LE(sizeof(NativeT) * count, + backing_buffer.GetArrayBuffer()->ByteLength() - byte_offset); + buffer_ = reinterpret_cast( const_cast(backing_buffer.GetNativeBuffer() + byte_offset)); @@ -75,17 +79,17 @@ class AliasedBuffer { js_array_ = v8::Global(isolate, js_array); } - AliasedBuffer(const AliasedBuffer& that) : - isolate_(that.isolate_), - count_(that.count_), - byte_offset_(that.byte_offset_), - buffer_(that.buffer_), - free_buffer_(false) { + AliasedBuffer(const AliasedBuffer& that) + : isolate_(that.isolate_), + count_(that.count_), + byte_offset_(that.byte_offset_), + buffer_(that.buffer_), + free_buffer_(false) { js_array_ = v8::Global(that.isolate_, that.GetJSArray()); } ~AliasedBuffer() { - if (free_buffer_ && buffer_ != NULL) { + if (free_buffer_ && buffer_ != nullptr) { free(buffer_); } js_array_.Reset(); @@ -97,14 +101,14 @@ class AliasedBuffer { */ class Reference { public: - Reference(AliasedBuffer* aliased_buffer, size_t index) : - aliased_buffer_(aliased_buffer), - index_(index) { + Reference(AliasedBuffer* aliased_buffer, size_t index) + : aliased_buffer_(aliased_buffer), + index_(index) { } - Reference(const Reference& that) : - aliased_buffer_(that.aliased_buffer_), - index_(that.index_) { + Reference(const Reference& that) + : aliased_buffer_(that.aliased_buffer_), + index_(that.index_) { } inline Reference& operator=(const NativeT &val) { @@ -155,7 +159,9 @@ class AliasedBuffer { * Set position index to given value. */ inline void SetValue(const size_t index, NativeT value) { - CHECK_LT(index, count_); + #if defined(DEBUG) && DEBUG + CHECK_LT(index, count_); + #endif buffer_[index] = value; } @@ -163,7 +169,9 @@ class AliasedBuffer { * Get value at position index */ inline const NativeT GetValue(const size_t index) const { - CHECK_LT(index, count_); + #if defined(DEBUG) && DEBUG + CHECK_LT(index, count_); + #endif return buffer_[index]; } diff --git a/test/cctest/test_aliased_buffer.cc b/test/cctest/test_aliased_buffer.cc index 500b4da485fb7e..65b029f2a02d02 100644 --- a/test/cctest/test_aliased_buffer.cc +++ b/test/cctest/test_aliased_buffer.cc @@ -24,53 +24,51 @@ void CreateOracleValues(NativeT* buf, size_t count) { } template -void WriteViaOperator( - AliasedBuffer& aliasedBuffer, // NOLINT(runtime/references) - size_t size, - NativeT* oracle) { +void WriteViaOperator(AliasedBuffer* aliasedBuffer, + size_t size, + NativeT* oracle) { // write through the API for (size_t i = 0; i < size; i++) { - aliasedBuffer[i] = oracle[i]; + (*aliasedBuffer)[i] = oracle[i]; } } template -void WriteViaSetValue( - AliasedBuffer& aliasedBuffer, // NOLINT(runtime/references) - size_t size, NativeT* oracle) { +void WriteViaSetValue(AliasedBuffer* aliasedBuffer, + size_t size, + NativeT* oracle) { // write through the API for (size_t i = 0; i < size; i++) { - aliasedBuffer.SetValue(i, oracle[i]); + aliasedBuffer->SetValue(i, oracle[i]); } } template -void ReadAndValidate( - v8::Isolate* isolate, - v8::Local context, - AliasedBuffer& aliasedBuffer, // NOLINT(runtime/references) - size_t size, - NativeT* oracle) { +void ReadAndValidate(v8::Isolate* isolate, + v8::Local context, + AliasedBuffer* aliasedBuffer, + size_t size, + NativeT* oracle) { // read through the API for (size_t i = 0; i < size; i++) { - NativeT v1 = aliasedBuffer[i]; - NativeT v2 = aliasedBuffer.GetValue(i); + NativeT v1 = (*aliasedBuffer)[i]; + NativeT v2 = aliasedBuffer->GetValue(i); EXPECT_TRUE(v1 == oracle[i]); EXPECT_TRUE(v2 == oracle[i]); } // validate size of JS Buffer - EXPECT_TRUE(aliasedBuffer.GetJSArray()->Length() == size); + EXPECT_TRUE(aliasedBuffer->GetJSArray()->Length() == size); EXPECT_TRUE( - aliasedBuffer.GetJSArray()->ByteLength() == + aliasedBuffer->GetJSArray()->ByteLength() == (size * sizeof(NativeT))); // validate operator * and GetBuffer are the same - EXPECT_TRUE(aliasedBuffer.GetNativeBuffer() == *aliasedBuffer); + EXPECT_TRUE(aliasedBuffer->GetNativeBuffer() == *(*aliasedBuffer)); // read through the JS API for (size_t i = 0; i < size; i++) { - v8::Local v8TypedArray = aliasedBuffer.GetJSArray(); + v8::Local v8TypedArray = aliasedBuffer->GetJSArray(); v8::MaybeLocal v = v8TypedArray->Get(context, i); EXPECT_TRUE(v.IsEmpty() == false); v8::Local v2 = v.ToLocalChecked(); @@ -92,17 +90,17 @@ void ReadWriteTest(v8::Isolate* isolate) { AliasedBuffer ab(isolate, size); NativeT* oracle = new NativeT[size]; CreateOracleValues(oracle, size); - WriteViaOperator(ab, size, oracle); - ReadAndValidate(isolate, context, ab, size, oracle); + WriteViaOperator(&ab, size, oracle); + ReadAndValidate(isolate, context, &ab, size, oracle); - WriteViaSetValue(ab, size, oracle); + WriteViaSetValue(&ab, size, oracle); // validate copy constructor { AliasedBuffer ab2(ab); - ReadAndValidate(isolate, context, ab2, size, oracle); + ReadAndValidate(isolate, context, &ab2, size, oracle); } - ReadAndValidate(isolate, context, ab, size, oracle); + ReadAndValidate(isolate, context, &ab, size, oracle); delete[] oracle; } @@ -140,21 +138,21 @@ void SharedBufferTest( CreateOracleValues(oracle_B, count_B); CreateOracleValues(oracle_C, count_C); - WriteViaOperator(ab_A, count_A, oracle_A); - WriteViaOperator(ab_B, count_B, oracle_B); - WriteViaOperator(ab_C, count_C, oracle_C); + WriteViaOperator(&ab_A, count_A, oracle_A); + WriteViaOperator(&ab_B, count_B, oracle_B); + WriteViaOperator(&ab_C, count_C, oracle_C); - ReadAndValidate(isolate, context, ab_A, count_A, oracle_A); - ReadAndValidate(isolate, context, ab_B, count_B, oracle_B); - ReadAndValidate(isolate, context, ab_C, count_C, oracle_C); + ReadAndValidate(isolate, context, &ab_A, count_A, oracle_A); + ReadAndValidate(isolate, context, &ab_B, count_B, oracle_B); + ReadAndValidate(isolate, context, &ab_C, count_C, oracle_C); - WriteViaSetValue(ab_A, count_A, oracle_A); - WriteViaSetValue(ab_B, count_B, oracle_B); - WriteViaSetValue(ab_C, count_C, oracle_C); + WriteViaSetValue(&ab_A, count_A, oracle_A); + WriteViaSetValue(&ab_B, count_B, oracle_B); + WriteViaSetValue(&ab_C, count_C, oracle_C); - ReadAndValidate(isolate, context, ab_A, count_A, oracle_A); - ReadAndValidate(isolate, context, ab_B, count_B, oracle_B); - ReadAndValidate(isolate, context, ab_C, count_C, oracle_C); + ReadAndValidate(isolate, context, &ab_A, count_A, oracle_A); + ReadAndValidate(isolate, context, &ab_B, count_B, oracle_B); + ReadAndValidate(isolate, context, &ab_C, count_C, oracle_C); } TEST_F(AliasBufferTest, Uint8Array) { From c6508f756564354d48d5680d76934d920450c012 Mon Sep 17 00:00:00 2001 From: Mike Kaufman Date: Tue, 12 Sep 2017 17:58:08 -0700 Subject: [PATCH 4/4] respnoding to PR feedback --- src/aliased_buffer.h | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/aliased_buffer.h b/src/aliased_buffer.h index 1a6ebe2e3fd5ae..2e5598b4757902 100644 --- a/src/aliased_buffer.h +++ b/src/aliased_buffer.h @@ -65,16 +65,17 @@ class AliasedBuffer { byte_offset_(byte_offset), free_buffer_(false) { const v8::HandleScope handle_scope(isolate_); + + v8::Local ab = backing_buffer.GetArrayBuffer(); + // validate that the byte_offset is aligned with sizeof(NativeT) CHECK_EQ(byte_offset & (sizeof(NativeT) - 1), 0); // validate this fits inside the backing buffer - CHECK_LE(sizeof(NativeT) * count, - backing_buffer.GetArrayBuffer()->ByteLength() - byte_offset); + CHECK_LE(sizeof(NativeT) * count, ab->ByteLength() - byte_offset); buffer_ = reinterpret_cast( const_cast(backing_buffer.GetNativeBuffer() + byte_offset)); - v8::Local ab = backing_buffer.GetArrayBuffer(); v8::Local js_array = V8T::New(ab, byte_offset, count); js_array_ = v8::Global(isolate, js_array); } @@ -159,9 +160,9 @@ class AliasedBuffer { * Set position index to given value. */ inline void SetValue(const size_t index, NativeT value) { - #if defined(DEBUG) && DEBUG - CHECK_LT(index, count_); - #endif +#if defined(DEBUG) && DEBUG + CHECK_LT(index, count_); +#endif buffer_[index] = value; } @@ -169,9 +170,9 @@ class AliasedBuffer { * Get value at position index */ inline const NativeT GetValue(const size_t index) const { - #if defined(DEBUG) && DEBUG - CHECK_LT(index, count_); - #endif +#if defined(DEBUG) && DEBUG + CHECK_LT(index, count_); +#endif return buffer_[index]; }