Skip to content

buffer: auto random fill Buffer(num) and new Buffer(num) #11806

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

Closed
jasnell wants to merge 3 commits into from
Closed

buffer: auto random fill Buffer(num) and new Buffer(num) #11806

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
12cf538
buffer: random fill when using Buffer(num)
jasnell Mar 11, 2017
53b1253
src: add --pending-deprecation and NODE_PENDING_DEPRECATION
jasnell Mar 11, 2017
e89e556
buffer: add pending deprecation warning
jasnell Mar 11, 2017
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
Next Next commit
buffer: random fill when using Buffer(num)
  • Loading branch information
@jasnell
jasnell committed Mar 12, 2017
commit 12cf538bb3789ebece09ed95782539aa439d9a13
12 changes: 11 additions & 1 deletion lib/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,15 @@ function createUnsafeArrayBuffer(size) {
}
}

function createRandomFillBuffer(size) {
zeroFill[0] = 3;
try {
return new ArrayBuffer(size);
} finally {
zeroFill[0] = 1;
}
}

function createPool() {
poolSize = Buffer.poolSize;
allocPool = createUnsafeArrayBuffer(poolSize);
Expand Down Expand Up @@ -103,7 +112,8 @@ function Buffer(arg, encodingOrOffset, length) {
'If encoding is specified then the first argument must be a string'
);
}
return Buffer.allocUnsafe(arg);
assertSize(arg);
return new FastBuffer(createRandomFillBuffer(arg));

@addaleax addaleax Mar 11, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Part of the performance hit might be that this stops using pooled allocation. Is that intentional?

@jasnell jasnell Mar 12, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, but it needn't I suppose. This was largely just to keep it simple and consistent. If we happen to pull off the pooled allocation, do you imagine we would just zero fill or use the randomly selected value to fill?

@addaleax addaleax Mar 12, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@jasnell The best way that comes to mind for me right now is to have separate pools for uninitialized & for random-filled Buffers … alternatively, we could just keep using Buffer.allocUnsafe() here and fill manually.

@jasnell jasnell Mar 12, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah. I'm leaning in that direction to be honest (specifically, using Buffer.alloc(size, fill)). It would be much less disruptive and wouldn't require the underlying c/c++ change. I did it this way first to see what the perf would be like and it is way too much of a hit

@jasnell jasnell Mar 12, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm going to open a second PR that uses this other method. I'd like us to get some comparison benchmark runs on various platforms. Definitely think going with the alloc then fill in js is going to be the better option tho.

}
return Buffer.from(arg, encodingOrOffset, length);
}
Expand Down
11 changes: 8 additions & 3 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1036,10 +1036,15 @@ Local<Value> WinapiErrnoException(Isolate* isolate,


void* ArrayBufferAllocator::Allocate(size_t size) {
if (zero_fill_field_ || zero_fill_all_buffers)
if (zero_fill_all_buffers || zero_fill_field_ == 1) {
return node::UncheckedCalloc(size);
else
return node::UncheckedMalloc(size);
} else if (zero_fill_field_ == 3) {
void* mem = node::UncheckedMalloc(size);
if (mem != nullptr)
memset(mem, random_fill_value_, size);
return mem;
}
return node::UncheckedMalloc(size);
}

static bool DomainHasErrorHandler(const Environment* env,
Expand Down
6 changes: 6 additions & 0 deletions src/node_internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,11 @@ inline bool IsBigEndian() {

class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
public:
ArrayBufferAllocator() {
unsigned int seed = time(NULL);
random_fill_value_ = rand_r(&seed) % 256;
}

inline uint32_t* zero_fill_field() { return &zero_fill_field_; }

virtual void* Allocate(size_t size); // Defined in src/node.cc
Expand All @@ -205,6 +210,7 @@ class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {

private:
uint32_t zero_fill_field_ = 1; // Boolean but exposed as uint32 to JS land.
uint8_t random_fill_value_;
};

// Clear any domain and/or uncaughtException handlers to force the error's
Expand Down