-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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.
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
src: introduce PullAll method to speed up blob.text/arrayBuffer #49873
Changes from 2 commits
33e2913
eb086f2
6975d04
0f22ac7
d6cd738
7de4169
5eb1cd2
97c63a6
c496080
88488c3
8bc06eb
554bc6e
da89771
6575fac
9e04f2c
c954b18
cb4476b
5990034
41f56e9
c0826c7
af6db5c
92295eb
4d48632
2eb20e5
efb7614
a74d043
95b34c3
b36d25f
9f848d2
b3feb52
c18b114
0025722
aea6fa3
9c5d249
f5255f9
c6e16b1
1fe6ca2
8871e94
24e646b
de0939c
0751057
692fa20
a1da235
3ae48d7
8762899
2f830af
b4c1eb5
b8dc496
8062d42
7897fbe
3a5172e
b065bfa
ba272e1
f0bb380
8dc9233
b19350e
e9fdd1c
f18320c
739f0ab
5415139
890d831
b2d0b29
b20e7c8
8ef0c1c
cb8e195
9163bba
cf66f7d
ddaa3f6
e006c7a
637d6c5
0158bb8
385cfa2
49c7e45
98806b1
4b7fc22
65128b7
bc25ec1
f7ca69c
1f30134
e290ec0
d1b03e0
c1ed261
9edaaf7
b665c95
3bbb759
0475027
fd6d362
fa61f24
bce3251
3a105fe
ead31aa
6761dc2
8650351
7665ea7
3941379
672f793
e7a7779
c7a26dd
b8b8724
1656c5b
fe7c701
dfa47f0
cb854ac
98813e7
5585de4
9e155e8
3cc6e5a
8a6db3f
2b77d58
cf393e5
57903d8
cad9376
4853671
0be708e
063f689
78908e9
5346e73
1ffbb10
e9e025a
36929cb
86887f1
135b8f0
231e33e
acbcde3
1b11447
0912b61
3d4d114
2106839
88675bb
5eb529e
db6a0e2
3e717ec
fc00596
fd233ea
b8c972b
e87cbf5
42f1e3a
fe3a1e3
5a743ff
920e206
30200cb
d4924e4
cd7da8c
cbcdb50
d48ba77
edce6ae
410e6b8
cc4505f
eda59e7
a95b2d2
a7a5db3
7b537c7
87632af
cb454a9
735a80d
8b4a14d
cd32d73
b27b3a8
4eda24d
0345207
e41fa7f
418416a
54c1138
d1878d3
d8ddf86
feae47a
128aae4
d75f4f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -291,6 +291,7 @@ Local<FunctionTemplate> Blob::Reader::GetConstructorTemplate(Environment* env) { | |
BaseObject::kInternalFieldCount); | ||
tmpl->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "BlobReader")); | ||
SetProtoMethod(env->isolate(), tmpl, "pull", Pull); | ||
SetProtoMethod(env->isolate(), tmpl, "pullAll", PullAll); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also update typescript typings for blob? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updating |
||
env->set_blob_reader_constructor_template(tmpl); | ||
} | ||
return tmpl; | ||
|
@@ -379,6 +380,94 @@ void Blob::Reader::Pull(const FunctionCallbackInfo<Value>& args) { | |
std::move(next), node::bob::OPTIONS_END, nullptr, 0)); | ||
} | ||
|
||
void Blob::Reader::PullAll(const FunctionCallbackInfo<Value>& args) { | ||
Environment* env = Environment::GetCurrent(args); | ||
Blob::Reader* reader; | ||
ASSIGN_OR_RETURN_UNWRAP(&reader, args.Holder()); | ||
|
||
CHECK(args[0]->IsFunction()); | ||
Local<Function> fn = args[0].As<Function>(); | ||
CHECK(!fn->IsConstructor()); | ||
|
||
if (reader->eos_) { | ||
Local<Value> arg = Int32::New(env->isolate(), bob::STATUS_EOS); | ||
reader->MakeCallback(fn, 1, &arg); | ||
return args.GetReturnValue().Set(bob::STATUS_EOS); | ||
debadree25 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
struct View { | ||
std::shared_ptr<BackingStore> store; | ||
size_t length; | ||
size_t offset = 0; | ||
}; | ||
|
||
struct Impl { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use a little more descriptive struct names? |
||
BaseObjectPtr<Blob::Reader> reader; | ||
Global<Function> callback; | ||
Environment* env; | ||
size_t total = 0; | ||
std::vector<View> views; | ||
int status = 1; | ||
}; | ||
|
||
Impl* impl = new Impl(); | ||
impl->reader = BaseObjectPtr<Blob::Reader>(reader); | ||
impl->callback.Reset(env->isolate(), fn); | ||
impl->env = env; | ||
|
||
auto next = [impl](int status, | ||
const DataQueue::Vec* vecs, | ||
size_t count, | ||
bob::Done doneCb) mutable { | ||
Environment* env = impl->env; | ||
if (status == bob::STATUS_EOS) impl->reader->eos_ = true; | ||
|
||
if (count > 0) { | ||
// Copy the returns vectors into a single ArrayBuffer. | ||
size_t total = 0; | ||
for (size_t n = 0; n < count; n++) total += vecs[n].len; | ||
|
||
std::shared_ptr<BackingStore> store = | ||
v8::ArrayBuffer::NewBackingStore(env->isolate(), total); | ||
auto ptr = static_cast<uint8_t*>(store->Data()); | ||
for (size_t n = 0; n < count; n++) { | ||
std::copy(vecs[n].base, vecs[n].base + vecs[n].len, ptr); | ||
ptr += vecs[n].len; | ||
} | ||
// Since we copied the data buffers, signal that we're done with them. | ||
std::move(doneCb)(0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I personnally do not understand why there is a std::move... If it is correct, I recommend adding a comment to explain it. |
||
impl->views.push_back(View{store, total}); | ||
debadree25 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
impl->total += total; | ||
} | ||
|
||
impl->status = status; | ||
return; | ||
}; | ||
|
||
while (impl->status > 0) { | ||
impl->reader->inner_->Pull(next, node::bob::OPTIONS_END, nullptr, 0); | ||
debadree25 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
|
||
std::shared_ptr<BackingStore> store = | ||
ArrayBuffer::NewBackingStore(env->isolate(), impl->total); | ||
auto ptr = static_cast<uint8_t*>(store->Data()); | ||
for (size_t n = 0; n < impl->views.size(); n++) { | ||
debadree25 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
uint8_t* from = static_cast<uint8_t*>(impl->views[n].store->Data()) + | ||
impl->views[n].offset; | ||
std::copy(from, from + impl->views[n].length, ptr); | ||
ptr += impl->views[n].length; | ||
} | ||
|
||
Local<Value> argv[2] = { | ||
Int32::New(env->isolate(), impl->status), | ||
ArrayBuffer::New(env->isolate(), store), | ||
}; | ||
|
||
impl->reader->MakeCallback(fn, arraysize(argv), argv); | ||
auto dropMe = std::unique_ptr<Impl>(impl); | ||
args.GetReturnValue().Set(impl->status); | ||
} | ||
|
||
BaseObjectPtr<BaseObject> | ||
Blob::BlobTransferData::Deserialize( | ||
Environment* env, | ||
|
@@ -560,6 +649,7 @@ void Blob::RegisterExternalReferences(ExternalReferenceRegistry* registry) { | |
registry->Register(Blob::GetDataObject); | ||
registry->Register(Blob::RevokeObjectURL); | ||
registry->Register(Blob::Reader::Pull); | ||
registry->Register(Blob::Reader::PullAll); | ||
registry->Register(Concat); | ||
registry->Register(BlobFromFilePath); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you pass resolve and reject methods to
pullAll
function, we don't even need to return anything and just handle it on C++ side. It might provide a performance boost. This would also reduce calling and mutating the callback function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea!