Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

buffer: construct Buffers in JS #2866

Closed
wants to merge 1 commit into from
Closed

buffer: construct Buffers in JS #2866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions lib/buffer.js
View file
Open in desktop
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;
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about making this one-byte Uint8Array instead, and passing it to the C++?

Copy link
Member

Choose a reason for hiding this comment

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

nvm, you did it this way :)

const kNoZeroFill = 0;


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

Choose a reason for hiding this comment

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

Hahaha, this one is better! :)

Are you afraid of class...extends? We can address it later, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

class is too slow. The call to super() can't be optimized by v8. I learned this b/c the initial implementation used class...extends and I never saw the constructor show up in IRHydra.

Copy link
Member

Choose a reason for hiding this comment

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

You sure about this? I have seen the disassembly output, and it looked like the Buffer constructor was inlined and was called Uint8Array constructor directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was when class was very first supported. So additional optimizations may have been done since then.

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
View file
Open in desktop
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::ArrayBufferAllocator::ArrayBufferAllocator() {
for (int i = 0; i < kFieldsCount; ++i)
fields_[i] = 0;
}

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

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

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

inline void Environment::ArrayBufferAllocator::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::ArrayBufferAllocator*
Environment::array_buffer_allocator() {
return &array_buffer_allocator_;
}

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

class ArrayBufferAllocator {
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 ArrayBufferAllocator();

enum Fields {
kNoZeroFill,
kFieldsCount
};

uint32_t fields_[kFieldsCount];

DISALLOW_COPY_AND_ASSIGN(ArrayBufferAllocator);
};

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 ArrayBufferAllocator* array_buffer_allocator();
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_;
ArrayBufferAllocator array_buffer_allocator_;
Copy link
Member

Choose a reason for hiding this comment

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

Who sets this thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't understand. this class is setup like all the other instance utility classes in Environment.

Copy link
Member

Choose a reason for hiding this comment

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

Mmm.. but it will be allocated twice now? Once here, and once below when you do new ArrayBufferAllocator

Copy link
Member

Choose a reason for hiding this comment

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

Wait, are those two different? Argh...

Copy link
Member

Choose a reason for hiding this comment

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

Let's call it ArrayBufferAllocatorInfo for gods sake :) I can imagine many people will be confused over this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hah! sounds good. :)

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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -890,6 +890,14 @@ Local<Value> WinapiErrnoException(Isolate* isolate,
#endif


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


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

Expand Down Expand Up @@ -3875,8 +3883,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();
Copy link
Member

Choose a reason for hiding this comment

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

Who is going to destroy this thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gah. This is remnants of two different implementations. It shouldn't be instantiated this way. The entire thing can be a struct in Environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

partial nm on that. it's cleaned up below after the isolate has been disposed.

params.array_buffer_allocator = array_buffer_allocator;
Isolate* isolate = Isolate::New(params);
if (track_heap_objects) {
isolate->GetHeapProfiler()->StartTrackingHeapObjects(true);
Expand All @@ -3892,6 +3900,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 @@ -3938,13 +3947,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
51 changes: 13 additions & 38 deletions src/node_buffer.cc
View file
Open in desktop
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 @@ -989,6 +953,18 @@ 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()->fields();
uint32_t const fields_count = env->array_buffer_allocator()->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 @@ -998,7 +974,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
View file
Open in desktop
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