From 5026db142ab5d07c7144dfb272512d3388476a77 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 26 May 2019 22:34:51 +0200 Subject: [PATCH] src: use ArrayBufferViewContents more frequently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Using `ArrayBufferViewContents` over `Buffer::Data()`/`Buffer::Length()` or `SPREAD_BUFFER_ARG` has the advantages of creating fewer individual variables to keep track off, not being a “magic” macro that creates variables, reducing code size, and being faster when receiving on-heap TypedArrays. PR-URL: https://github.com/nodejs/node/pull/27920 Reviewed-By: Ben Noordhuis Reviewed-By: Sam Roberts Reviewed-By: Colin Ihrig --- src/js_stream.cc | 6 +-- src/node_buffer.cc | 80 ++++++++++++++++++------------------- src/node_crypto.cc | 27 +++++-------- src/node_http_parser_impl.h | 13 +++--- src/node_i18n.cc | 14 +++---- src/node_serdes.cc | 4 +- src/tls_wrap.cc | 6 +-- src/util-inl.h | 7 ++++ src/util.h | 1 + 9 files changed, 77 insertions(+), 81 deletions(-) diff --git a/src/js_stream.cc b/src/js_stream.cc index 1d61605d645..2663106ba7a 100644 --- a/src/js_stream.cc +++ b/src/js_stream.cc @@ -167,9 +167,9 @@ void JSStream::ReadBuffer(const FunctionCallbackInfo& args) { JSStream* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); - CHECK(Buffer::HasInstance(args[0])); - char* data = Buffer::Data(args[0]); - int len = Buffer::Length(args[0]); + ArrayBufferViewContents buffer(args[0]); + const char* data = buffer.data(); + int len = buffer.length(); // Repeatedly ask the stream's owner for memory, copy the data that we // just read from JS into those buffers and emit them as reads. diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 3b4be5a8105..e6a88f64989 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -463,17 +463,17 @@ void StringSlice(const FunctionCallbackInfo& args) { Isolate* isolate = env->isolate(); THROW_AND_RETURN_UNLESS_BUFFER(env, args.This()); - SPREAD_BUFFER_ARG(args.This(), ts_obj); + ArrayBufferViewContents buffer(args.This()); - if (ts_obj_length == 0) + if (buffer.length() == 0) return args.GetReturnValue().SetEmptyString(); - SLICE_START_END(env, args[0], args[1], ts_obj_length) + SLICE_START_END(env, args[0], args[1], buffer.length()) Local error; MaybeLocal ret = StringBytes::Encode(isolate, - ts_obj_data + start, + buffer.data() + start, length, encoding, &error); @@ -492,9 +492,8 @@ void Copy(const FunctionCallbackInfo &args) { THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]); THROW_AND_RETURN_UNLESS_BUFFER(env, args[1]); - Local buffer_obj = args[0].As(); + ArrayBufferViewContents source(args[0]); Local target_obj = args[1].As(); - SPREAD_BUFFER_ARG(buffer_obj, ts_obj); SPREAD_BUFFER_ARG(target_obj, target); size_t target_start = 0; @@ -503,14 +502,14 @@ void Copy(const FunctionCallbackInfo &args) { THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[2], 0, &target_start)); THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[3], 0, &source_start)); - THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[4], ts_obj_length, + THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[4], source.length(), &source_end)); // Copy 0 bytes; we're done if (target_start >= target_length || source_start >= source_end) return args.GetReturnValue().Set(0); - if (source_start > ts_obj_length) + if (source_start > source.length()) return THROW_ERR_OUT_OF_RANGE( env, "The value of \"sourceStart\" is out of range."); @@ -519,9 +518,9 @@ void Copy(const FunctionCallbackInfo &args) { uint32_t to_copy = std::min( std::min(source_end - source_start, target_length - target_start), - ts_obj_length - source_start); + source.length() - source_start); - memmove(target_data + target_start, ts_obj_data + source_start, to_copy); + memmove(target_data + target_start, source.data() + source_start, to_copy); args.GetReturnValue().Set(to_copy); } @@ -689,8 +688,8 @@ void CompareOffset(const FunctionCallbackInfo &args) { THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]); THROW_AND_RETURN_UNLESS_BUFFER(env, args[1]); - SPREAD_BUFFER_ARG(args[0], ts_obj); - SPREAD_BUFFER_ARG(args[1], target); + ArrayBufferViewContents source(args[0]); + ArrayBufferViewContents target(args[1]); size_t target_start = 0; size_t source_start = 0; @@ -699,15 +698,15 @@ void CompareOffset(const FunctionCallbackInfo &args) { THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[2], 0, &target_start)); THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[3], 0, &source_start)); - THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[4], target_length, + THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[4], target.length(), &target_end)); - THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[5], ts_obj_length, + THROW_AND_RETURN_IF_OOB(ParseArrayIndex(env, args[5], source.length(), &source_end)); - if (source_start > ts_obj_length) + if (source_start > source.length()) return THROW_ERR_OUT_OF_RANGE( env, "The value of \"sourceStart\" is out of range."); - if (target_start > target_length) + if (target_start > target.length()) return THROW_ERR_OUT_OF_RANGE( env, "The value of \"targetStart\" is out of range."); @@ -716,11 +715,11 @@ void CompareOffset(const FunctionCallbackInfo &args) { size_t to_cmp = std::min(std::min(source_end - source_start, target_end - target_start), - ts_obj_length - source_start); + source.length() - source_start); int val = normalizeCompareVal(to_cmp > 0 ? - memcmp(ts_obj_data + source_start, - target_data + target_start, + memcmp(source.data() + source_start, + target.data() + target_start, to_cmp) : 0, source_end - source_start, target_end - target_start); @@ -733,14 +732,14 @@ void Compare(const FunctionCallbackInfo &args) { THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]); THROW_AND_RETURN_UNLESS_BUFFER(env, args[1]); - SPREAD_BUFFER_ARG(args[0], obj_a); - SPREAD_BUFFER_ARG(args[1], obj_b); + ArrayBufferViewContents a(args[0]); + ArrayBufferViewContents b(args[1]); - size_t cmp_length = std::min(obj_a_length, obj_b_length); + size_t cmp_length = std::min(a.length(), b.length()); int val = normalizeCompareVal(cmp_length > 0 ? - memcmp(obj_a_data, obj_b_data, cmp_length) : 0, - obj_a_length, obj_b_length); + memcmp(a.data(), b.data(), cmp_length) : 0, + a.length(), b.length()); args.GetReturnValue().Set(val); } @@ -792,16 +791,16 @@ void IndexOfString(const FunctionCallbackInfo& args) { enum encoding enc = ParseEncoding(isolate, args[3], UTF8); THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]); - SPREAD_BUFFER_ARG(args[0], ts_obj); + ArrayBufferViewContents buffer(args[0]); Local needle = args[1].As(); int64_t offset_i64 = args[2].As()->Value(); bool is_forward = args[4]->IsTrue(); - const char* haystack = ts_obj_data; + const char* haystack = buffer.data(); // Round down to the nearest multiple of 2 in case of UCS2. const size_t haystack_length = (enc == UCS2) ? - ts_obj_length &~ 1 : ts_obj_length; // NOLINT(whitespace/operators) + buffer.length() &~ 1 : buffer.length(); // NOLINT(whitespace/operators) size_t needle_length; if (!StringBytes::Size(isolate, needle, enc).To(&needle_length)) return; @@ -909,15 +908,15 @@ void IndexOfBuffer(const FunctionCallbackInfo& args) { THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]); THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[1]); - SPREAD_BUFFER_ARG(args[0], ts_obj); - SPREAD_BUFFER_ARG(args[1], buf); + ArrayBufferViewContents haystack_contents(args[0]); + ArrayBufferViewContents needle_contents(args[1]); int64_t offset_i64 = args[2].As()->Value(); bool is_forward = args[4]->IsTrue(); - const char* haystack = ts_obj_data; - const size_t haystack_length = ts_obj_length; - const char* needle = buf_data; - const size_t needle_length = buf_length; + const char* haystack = haystack_contents.data(); + const size_t haystack_length = haystack_contents.length(); + const char* needle = needle_contents.data(); + const size_t needle_length = needle_contents.length(); int64_t opt_offset = IndexOfOffset(haystack_length, offset_i64, @@ -978,27 +977,28 @@ void IndexOfNumber(const FunctionCallbackInfo& args) { CHECK(args[3]->IsBoolean()); THROW_AND_RETURN_UNLESS_BUFFER(Environment::GetCurrent(args), args[0]); - SPREAD_BUFFER_ARG(args[0], ts_obj); + ArrayBufferViewContents buffer(args[0]); uint32_t needle = args[1].As()->Value(); int64_t offset_i64 = args[2].As()->Value(); bool is_forward = args[3]->IsTrue(); - int64_t opt_offset = IndexOfOffset(ts_obj_length, offset_i64, 1, is_forward); - if (opt_offset <= -1 || ts_obj_length == 0) { + int64_t opt_offset = + IndexOfOffset(buffer.length(), offset_i64, 1, is_forward); + if (opt_offset <= -1 || buffer.length() == 0) { return args.GetReturnValue().Set(-1); } size_t offset = static_cast(opt_offset); - CHECK_LT(offset, ts_obj_length); + CHECK_LT(offset, buffer.length()); const void* ptr; if (is_forward) { - ptr = memchr(ts_obj_data + offset, needle, ts_obj_length - offset); + ptr = memchr(buffer.data() + offset, needle, buffer.length() - offset); } else { - ptr = node::stringsearch::MemrchrFill(ts_obj_data, needle, offset + 1); + ptr = node::stringsearch::MemrchrFill(buffer.data(), needle, offset + 1); } const char* ptr_char = static_cast(ptr); - args.GetReturnValue().Set(ptr ? static_cast(ptr_char - ts_obj_data) + args.GetReturnValue().Set(ptr ? static_cast(ptr_char - buffer.data()) : -1); } diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 76a8cc8a193..a5710dc33b6 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5766,11 +5766,10 @@ void ECDH::SetPrivateKey(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&ecdh, args.Holder()); THROW_AND_RETURN_IF_NOT_BUFFER(env, args[0], "Private key"); + ArrayBufferViewContents priv_buffer(args[0]); BignumPointer priv(BN_bin2bn( - reinterpret_cast(Buffer::Data(args[0].As())), - Buffer::Length(args[0].As()), - nullptr)); + priv_buffer.data(), priv_buffer.length(), nullptr)); if (!priv) return env->ThrowError("Failed to convert Buffer to BN"); @@ -6687,14 +6686,11 @@ OpenSSLBuffer ExportChallenge(const char* data, int len) { void ExportChallenge(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - size_t len = Buffer::Length(args[0]); - if (len == 0) + ArrayBufferViewContents input(args[0]); + if (input.length() == 0) return args.GetReturnValue().SetEmptyString(); - char* data = Buffer::Data(args[0]); - CHECK_NOT_NULL(data); - - OpenSSLBuffer cert = ExportChallenge(data, len); + OpenSSLBuffer cert = ExportChallenge(input.data(), input.length()); if (!cert) return args.GetReturnValue().SetEmptyString(); @@ -6749,16 +6745,13 @@ void ConvertKey(const FunctionCallbackInfo& args) { void TimingSafeEqual(const FunctionCallbackInfo& args) { - CHECK(Buffer::HasInstance(args[0])); - CHECK(Buffer::HasInstance(args[1])); + ArrayBufferViewContents buf1(args[0]); + ArrayBufferViewContents buf2(args[1]); - size_t buf_length = Buffer::Length(args[0]); - CHECK_EQ(buf_length, Buffer::Length(args[1])); + CHECK_EQ(buf1.length(), buf2.length()); - const char* buf1 = Buffer::Data(args[0]); - const char* buf2 = Buffer::Data(args[1]); - - return args.GetReturnValue().Set(CRYPTO_memcmp(buf1, buf2, buf_length) == 0); + return args.GetReturnValue().Set( + CRYPTO_memcmp(buf1.data(), buf2.data(), buf1.length()) == 0); } void InitCryptoOnce() { diff --git a/src/node_http_parser_impl.h b/src/node_http_parser_impl.h index a354c6fcc51..1afedb0509b 100644 --- a/src/node_http_parser_impl.h +++ b/src/node_http_parser_impl.h @@ -466,18 +466,15 @@ class Parser : public AsyncWrap, public StreamListener { CHECK(parser->current_buffer_.IsEmpty()); CHECK_EQ(parser->current_buffer_len_, 0); CHECK_NULL(parser->current_buffer_data_); - CHECK_EQ(Buffer::HasInstance(args[0]), true); - Local buffer_obj = args[0].As(); - char* buffer_data = Buffer::Data(buffer_obj); - size_t buffer_len = Buffer::Length(buffer_obj); + ArrayBufferViewContents buffer(args[0]); // This is a hack to get the current_buffer to the callbacks with the least // amount of overhead. Nothing else will run while http_parser_execute() // runs, therefore this pointer can be set and used for the execution. - parser->current_buffer_ = buffer_obj; + parser->current_buffer_ = args[0].As(); - Local ret = parser->Execute(buffer_data, buffer_len); + Local ret = parser->Execute(buffer.data(), buffer.length()); if (!ret.IsEmpty()) args.GetReturnValue().Set(ret); @@ -643,7 +640,7 @@ class Parser : public AsyncWrap, public StreamListener { } - Local Execute(char* data, size_t len) { + Local Execute(const char* data, size_t len) { EscapableHandleScope scope(env()->isolate()); current_buffer_len_ = len; @@ -857,7 +854,7 @@ class Parser : public AsyncWrap, public StreamListener { bool got_exception_; Local current_buffer_; size_t current_buffer_len_; - char* current_buffer_data_; + const char* current_buffer_data_; #ifdef NODE_EXPERIMENTAL_HTTP unsigned int execute_depth_ = 0; bool pending_pause_ = false; diff --git a/src/node_i18n.cc b/src/node_i18n.cc index 8ccda35056c..ad5c8283924 100644 --- a/src/node_i18n.cc +++ b/src/node_i18n.cc @@ -205,14 +205,13 @@ class ConverterObject : public BaseObject, Converter { ConverterObject* converter; ASSIGN_OR_RETURN_UNWRAP(&converter, args[0].As()); - SPREAD_BUFFER_ARG(args[1], input_obj); + ArrayBufferViewContents input(args[1]); int flags = args[2]->Uint32Value(env->context()).ToChecked(); UErrorCode status = U_ZERO_ERROR; MaybeStackBuffer result; MaybeLocal ret; - size_t limit = ucnv_getMinCharSize(converter->conv) * - input_obj_length; + size_t limit = ucnv_getMinCharSize(converter->conv) * input.length(); if (limit > 0) result.AllocateSufficientStorage(limit); @@ -225,8 +224,8 @@ class ConverterObject : public BaseObject, Converter { } }); - const char* source = input_obj_data; - size_t source_length = input_obj_length; + const char* source = input.data(); + size_t source_length = input.length(); if (converter->unicode_ && !converter->ignoreBOM_ && !converter->bomSeen_) { int32_t bomOffset = 0; @@ -455,8 +454,7 @@ void Transcode(const FunctionCallbackInfo&args) { UErrorCode status = U_ZERO_ERROR; MaybeLocal result; - CHECK(Buffer::HasInstance(args[0])); - SPREAD_BUFFER_ARG(args[0], ts_obj); + ArrayBufferViewContents input(args[0]); const enum encoding fromEncoding = ParseEncoding(isolate, args[1], BUFFER); const enum encoding toEncoding = ParseEncoding(isolate, args[2], BUFFER); @@ -490,7 +488,7 @@ void Transcode(const FunctionCallbackInfo&args) { } result = tfn(env, EncodingName(fromEncoding), EncodingName(toEncoding), - ts_obj_data, ts_obj_length, &status); + input.data(), input.length(), &status); } else { status = U_ILLEGAL_ARGUMENT_ERROR; } diff --git a/src/node_serdes.cc b/src/node_serdes.cc index 41ee8afd8cb..a2d185c4167 100644 --- a/src/node_serdes.cc +++ b/src/node_serdes.cc @@ -274,8 +274,8 @@ void SerializerContext::WriteRawBytes(const FunctionCallbackInfo& args) { ctx->env(), "source must be a TypedArray or a DataView"); } - ctx->serializer_.WriteRawBytes(Buffer::Data(args[0]), - Buffer::Length(args[0])); + ArrayBufferViewContents bytes(args[0]); + ctx->serializer_.WriteRawBytes(bytes.data(), bytes.length()); } DeserializerContext::DeserializerContext(Environment* env, diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index cde5419b9c4..e9fe9693586 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -187,9 +187,9 @@ void TLSWrap::Receive(const FunctionCallbackInfo& args) { TLSWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); - CHECK(Buffer::HasInstance(args[0])); - char* data = Buffer::Data(args[0]); - size_t len = Buffer::Length(args[0]); + ArrayBufferViewContents buffer(args[0]); + const char* data = buffer.data(); + size_t len = buffer.length(); Debug(wrap, "Receiving %zu bytes injected from JS", len); // Copy given buffer entirely or partiall if handle becomes closed diff --git a/src/util-inl.h b/src/util-inl.h index 1abebf6e137..5f8e7acb2a8 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -495,6 +495,13 @@ ArrayBufferViewContents::ArrayBufferViewContents( Read(value.As()); } +template +ArrayBufferViewContents::ArrayBufferViewContents( + v8::Local value) { + CHECK(value->IsArrayBufferView()); + Read(value.As()); +} + template ArrayBufferViewContents::ArrayBufferViewContents( v8::Local abv) { diff --git a/src/util.h b/src/util.h index b7a2d28b669..a94e88f232f 100644 --- a/src/util.h +++ b/src/util.h @@ -446,6 +446,7 @@ class ArrayBufferViewContents { ArrayBufferViewContents() = default; explicit inline ArrayBufferViewContents(v8::Local value); + explicit inline ArrayBufferViewContents(v8::Local value); explicit inline ArrayBufferViewContents(v8::Local abv); inline void Read(v8::Local abv);