Skip to content

Commit

Permalink
buffer: construct Uint8Array in JS
Browse files Browse the repository at this point in the history
Overall construction time of Typed Arrays is faster in JS, but the
problem with using it normally is zero-fill of memory. Get around this
by using a flag in the ArrayBuffer::Allocator to trigger when memory
should or shouldn't be zero-filled.

Remove Buffer::Create() as it is no longer called.

The creation of the Uint8Array() was done at each callsite because at
the time of this patch there was a performance penalty for centralizing
the call in a single function.

PR-URL: #2866
Reviewed-By: Fedor Indutny <[email protected]>
  • Loading branch information
trevnorris committed Sep 15, 2015
1 parent 7d79412 commit 74178a5
Show file tree
Hide file tree
Showing 6 changed files with 107 additions and 61 deletions.
32 changes: 21 additions & 11 deletions lib/buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

const binding = process.binding('buffer');
const internalUtil = require('internal/util');
const bindingObj = {};

exports.Buffer = Buffer;
exports.SlowBuffer = SlowBuffer;
Expand All @@ -14,11 +15,19 @@ Buffer.poolSize = 8 * 1024;
var poolSize, poolOffset, allocPool;


binding.setupBufferJS(Buffer.prototype, bindingObj);
const flags = bindingObj.flags;
const kNoZeroFill = 0;


function createPool() {
poolSize = Buffer.poolSize;
allocPool = binding.create(poolSize);
flags[kNoZeroFill] = 1;
allocPool = new Uint8Array(poolSize);
Object.setPrototypeOf(allocPool, Buffer.prototype);
poolOffset = 0;
}
createPool();


function alignPool() {
Expand Down Expand Up @@ -46,30 +55,28 @@ function Buffer(arg) {

// Unusual.
return fromObject(arg);
};
}

Buffer.prototype.__proto__ = Uint8Array.prototype;
Buffer.__proto__ = Uint8Array;


binding.setupBufferJS(Buffer.prototype);
// Buffer prototype must be past before creating our first pool.
createPool();


function SlowBuffer(length) {
if (+length != length)
length = 0;
return binding.create(+length);
};
flags[kNoZeroFill] = 1;
const ui8 = new Uint8Array(+length);
Object.setPrototypeOf(ui8, Buffer.prototype);
return ui8;
}

SlowBuffer.prototype.__proto__ = Buffer.prototype;
SlowBuffer.__proto__ = Buffer;


function allocate(size) {
if (size === 0)
return binding.create(0);
return SlowBuffer(0);
if (size < (Buffer.poolSize >>> 1)) {
if (size > (poolSize - poolOffset))
createPool();
Expand All @@ -78,7 +85,10 @@ function allocate(size) {
alignPool();
return b;
} else {
return binding.create(size);
flags[kNoZeroFill] = 1;
const ui8 = new Uint8Array(size);
Object.setPrototypeOf(ui8, Buffer.prototype);
return ui8;
}
}

Expand Down
26 changes: 26 additions & 0 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,27 @@ inline void Environment::TickInfo::set_last_threw(bool value) {
last_threw_ = value;
}

inline Environment::ArrayBufferAllocatorInfo::ArrayBufferAllocatorInfo() {
for (int i = 0; i < kFieldsCount; ++i)
fields_[i] = 0;
}

inline uint32_t* Environment::ArrayBufferAllocatorInfo::fields() {
return fields_;
}

inline int Environment::ArrayBufferAllocatorInfo::fields_count() const {
return kFieldsCount;
}

inline bool Environment::ArrayBufferAllocatorInfo::no_zero_fill() const {
return fields_[kNoZeroFill] != 0;
}

inline void Environment::ArrayBufferAllocatorInfo::reset_fill_flag() {
fields_[kNoZeroFill] = 0;
}

inline Environment* Environment::New(v8::Local<v8::Context> context,
uv_loop_t* loop) {
Environment* env = new Environment(context, loop);
Expand Down Expand Up @@ -290,6 +311,11 @@ inline Environment::TickInfo* Environment::tick_info() {
return &tick_info_;
}

inline Environment::ArrayBufferAllocatorInfo*
Environment::array_buffer_allocator_info() {
return &array_buffer_allocator_info_;
}

inline uint64_t Environment::timer_base() const {
return timer_base_;
}
Expand Down
23 changes: 23 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,27 @@ class Environment {
DISALLOW_COPY_AND_ASSIGN(TickInfo);
};

class ArrayBufferAllocatorInfo {
public:
inline uint32_t* fields();
inline int fields_count() const;
inline bool no_zero_fill() const;
inline void reset_fill_flag();

private:
friend class Environment; // So we can call the constructor.
inline ArrayBufferAllocatorInfo();

enum Fields {
kNoZeroFill,
kFieldsCount
};

uint32_t fields_[kFieldsCount];

DISALLOW_COPY_AND_ASSIGN(ArrayBufferAllocatorInfo);
};

typedef void (*HandleCleanupCb)(Environment* env,
uv_handle_t* handle,
void* arg);
Expand Down Expand Up @@ -401,6 +422,7 @@ class Environment {
inline AsyncHooks* async_hooks();
inline DomainFlag* domain_flag();
inline TickInfo* tick_info();
inline ArrayBufferAllocatorInfo* array_buffer_allocator_info();
inline uint64_t timer_base() const;

static inline Environment* from_cares_timer_handle(uv_timer_t* handle);
Expand Down Expand Up @@ -510,6 +532,7 @@ class Environment {
AsyncHooks async_hooks_;
DomainFlag domain_flag_;
TickInfo tick_info_;
ArrayBufferAllocatorInfo array_buffer_allocator_info_;
const uint64_t timer_base_;
uv_timer_t cares_timer_handle_;
ares_channel cares_channel_;
Expand Down
15 changes: 13 additions & 2 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -894,6 +894,14 @@ Local<Value> WinapiErrnoException(Isolate* isolate,
#endif


void* ArrayBufferAllocator::Allocate(size_t size) {
if (env_ == nullptr || !env_->array_buffer_allocator_info()->no_zero_fill())
return calloc(size, 1);
env_->array_buffer_allocator_info()->reset_fill_flag();
return malloc(size);
}


void SetupDomainUse(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

Expand Down Expand Up @@ -3879,8 +3887,8 @@ Environment* CreateEnvironment(Isolate* isolate,
static void StartNodeInstance(void* arg) {
NodeInstanceData* instance_data = static_cast<NodeInstanceData*>(arg);
Isolate::CreateParams params;
ArrayBufferAllocator array_buffer_allocator;
params.array_buffer_allocator = &array_buffer_allocator;
ArrayBufferAllocator* array_buffer_allocator = new ArrayBufferAllocator();
params.array_buffer_allocator = array_buffer_allocator;
Isolate* isolate = Isolate::New(params);
if (track_heap_objects) {
isolate->GetHeapProfiler()->StartTrackingHeapObjects(true);
Expand All @@ -3896,6 +3904,7 @@ static void StartNodeInstance(void* arg) {
HandleScope handle_scope(isolate);
Local<Context> context = Context::New(isolate);
Environment* env = CreateEnvironment(isolate, context, instance_data);
array_buffer_allocator->set_env(env);
Context::Scope context_scope(context);
if (instance_data->is_main())
env->set_using_abort_on_uncaught_exc(abort_on_uncaught_exception);
Expand Down Expand Up @@ -3942,13 +3951,15 @@ static void StartNodeInstance(void* arg) {
__lsan_do_leak_check();
#endif

array_buffer_allocator->set_env(nullptr);
env->Dispose();
env = nullptr;
}

CHECK_NE(isolate, nullptr);
isolate->Dispose();
isolate = nullptr;
delete array_buffer_allocator;
if (instance_data->is_main())
node_isolate = nullptr;
}
Expand Down
52 changes: 14 additions & 38 deletions src/node_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ using v8::Object;
using v8::Persistent;
using v8::String;
using v8::Uint32;
using v8::Uint32Array;
using v8::Uint8Array;
using v8::Value;
using v8::WeakCallbackData;
Expand Down Expand Up @@ -392,43 +393,6 @@ MaybeLocal<Object> New(Environment* env, char* data, size_t length) {
}


void Create(const FunctionCallbackInfo<Value>& args) {
Isolate* isolate = args.GetIsolate();
Environment* env = Environment::GetCurrent(args);

CHECK(args[0]->IsNumber());

int64_t length = args[0]->IntegerValue();

if (length < 0 || length > kMaxLength) {
return env->ThrowRangeError("invalid Buffer length");
}

void* data;
if (length > 0) {
data = malloc(length);
if (data == nullptr) {
return env->ThrowRangeError(
"Buffer allocation failed - process out of memory");
}
} else {
data = nullptr;
}

Local<ArrayBuffer> ab =
ArrayBuffer::New(isolate,
data,
length,
ArrayBufferCreationMode::kInternalized);
Local<Uint8Array> ui = Uint8Array::New(ab, 0, length);
Maybe<bool> mb =
ui->SetPrototype(env->context(), env->buffer_prototype_object());
if (!mb.FromMaybe(false))
return env->ThrowError("Unable to set Object prototype");
args.GetReturnValue().Set(ui);
}


void CreateFromString(const FunctionCallbackInfo<Value>& args) {
CHECK(args[0]->IsString());
CHECK(args[1]->IsString());
Expand Down Expand Up @@ -966,6 +930,19 @@ void SetupBufferJS(const FunctionCallbackInfo<Value>& args) {
env->SetMethod(proto, "utf8Write", Utf8Write);

env->SetMethod(proto, "copy", Copy);

CHECK(args[1]->IsObject());
Local<Object> bObj = args[1].As<Object>();

uint32_t* const fields = env->array_buffer_allocator_info()->fields();
uint32_t const fields_count =
env->array_buffer_allocator_info()->fields_count();

Local<ArrayBuffer> array_buffer =
ArrayBuffer::New(env->isolate(), fields, sizeof(*fields) * fields_count);

bObj->Set(String::NewFromUtf8(env->isolate(), "flags"),
Uint32Array::New(array_buffer, 0, fields_count));
}


Expand All @@ -975,7 +952,6 @@ void Initialize(Local<Object> target,
Environment* env = Environment::GetCurrent(context);

env->SetMethod(target, "setupBufferJS", SetupBufferJS);
env->SetMethod(target, "create", Create);
env->SetMethod(target, "createFromString", CreateFromString);
env->SetMethod(target, "createFromArrayBuffer", CreateFromArrayBuffer);

Expand Down
20 changes: 10 additions & 10 deletions src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -228,18 +228,18 @@ NODE_DEPRECATED("Use ThrowUVException(isolate)",
return ThrowUVException(isolate, errorno, syscall, message, path);
})

struct ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
virtual void* Allocate(size_t size) {
return calloc(size, 1);
}
class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
public:
ArrayBufferAllocator() { }

virtual void* AllocateUninitialized(size_t size) {
return malloc(size);
}
inline void set_env(Environment* env) { env_ = env; }

virtual void Free(void* data, size_t) {
free(data);
}
virtual void* Allocate(size_t size); // Defined in src/node.cc
virtual void* AllocateUninitialized(size_t size) { return malloc(size); }
virtual void Free(void* data, size_t) { free(data); }

private:
Environment* env_;
};

enum NodeInstanceType { MAIN, WORKER };
Expand Down

10 comments on commit 74178a5

@indutny
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the IRC discussion: this thing will not work for small array buffers (<= 64 bytes, I think). They will end up allocated on v8 heap, and will be moved around. This is very very bad thing.

@nodejs/build : this should not land in 4.0.1, until we will fix it.

@indutny
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fishrock123
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, ok. should revert on master or only 4.x?

@Fishrock123
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny please cc @nodejs/release for this kind of thing :)

@indutny
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, too much WGs :) We should not let it go to 4.x, it is probably fine to have it in master for some time if we are not going to release it.

@trevnorris
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a simple fix that I almost finished before had to run to dinner. We just need to force --typed_array_max_size_in_heap=0 at startup.

@indutny
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevnorris this will work as a hot fix, but shouldn't we handle it ourselves afterwards?

@trevnorris
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny This is the solution recommended by @jeisinger: #1451 (comment)

I don't understand what you mean by "handle it ourselves".

@indutny
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevnorris we can allocate buffers using "the old way" for small sizes.

@trevnorris
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny This can fall short for those who want to "subclass" Buffer. Additionally, unless we scan for that flag, intercept, then store the passed value we can't know for sure that we should only use the old way for 64 byte sizes.

Please sign in to comment.