Skip to content

Commit af7582b

Browse files
jasnelljasnell
authored
Variation of jsg::Function that takes jgs::Arguments<jsg::Value> (#1029)
1 parent 53f0da1 commit af7582b

8 files changed

+114
-96
lines changed

‎src/workerd/api/global-scope.c++

+25-36
Original file line numberDiff line numberDiff line change
@@ -706,28 +706,21 @@ TimeoutId::NumberType ServiceWorkerGlobalScope::setTimeoutInternal(
706706

707707
TimeoutId::NumberType ServiceWorkerGlobalScope::setTimeout(
708708
jsg::Lock& js,
709-
jsg::V8Ref<v8::Function> function,
709+
jsg::Function<void(jsg::Arguments<jsg::Value>)> function,
710710
jsg::Optional<double> msDelay,
711-
jsg::Varargs args) {
712-
KJ_IF_MAYBE(context, jsg::AsyncContextFrame::current(js)) {
713-
function = js.v8Ref(context->wrap(js, function));
714-
}
715-
auto argv = kj::heapArrayFromIterable<jsg::Value>(kj::mv(args));
711+
jsg::Arguments<jsg::Value> args) {
712+
function.setReceiver(js.v8Ref<v8::Value>(js.v8Context()->Global()));
713+
auto fn = [function=kj::mv(function),
714+
args=kj::mv(args),
715+
context=jsg::AsyncContextFrame::currentRef(js)](jsg::Lock& js) mutable {
716+
jsg::AsyncContextFrame::Scope scope(js, context);
717+
function(js, kj::mv(args));
718+
};
716719
auto timeoutId = IoContext::current().setTimeoutImpl(
717720
timeoutIdGenerator,
718721
/* repeats = */ false,
719-
[function = function.addRef(js),
720-
argv = kj::mv(argv)]
721-
(jsg::Lock& js) mutable {
722-
auto context = js.v8Context();
723-
auto localFunction = function.getHandle(js);
724-
auto localArgs = KJ_MAP(arg, argv) {
725-
return arg.getHandle(js);
726-
};
727-
auto argc = localArgs.size();
728-
729-
// Cast to void to discard the result value.
730-
(void)jsg::check(localFunction->Call(context, context->Global(), argc, &localArgs.front()));
722+
[function = kj::mv(fn)](jsg::Lock& js) mutable {
723+
function(js);
731724
}, msDelay.orDefault(0));
732725
return timeoutId.toNumber();
733726
}
@@ -740,28 +733,24 @@ void ServiceWorkerGlobalScope::clearTimeout(kj::Maybe<TimeoutId::NumberType> tim
740733

741734
TimeoutId::NumberType ServiceWorkerGlobalScope::setInterval(
742735
jsg::Lock& js,
743-
jsg::V8Ref<v8::Function> function,
736+
jsg::Function<void(jsg::Arguments<jsg::Value>)> function,
744737
jsg::Optional<double> msDelay,
745-
jsg::Varargs args) {
746-
KJ_IF_MAYBE(context, jsg::AsyncContextFrame::current(js)) {
747-
function = js.v8Ref(context->wrap(js, function));
748-
}
749-
auto argv = kj::heapArrayFromIterable<jsg::Value>(kj::mv(args));
738+
jsg::Arguments<jsg::Value> args) {
739+
function.setReceiver(js.v8Ref<v8::Value>(js.v8Context()->Global()));
740+
auto fn = [function=kj::mv(function),
741+
args=kj::mv(args),
742+
context=jsg::AsyncContextFrame::currentRef(js)]
743+
(jsg::Lock& js) mutable {
744+
jsg::AsyncContextFrame::Scope scope(js, context);
745+
// Because the fn is called multiple times, we will clone the args on each call.
746+
auto argv = KJ_MAP(i, args) { return i.addRef(js); };
747+
function(js, jsg::Arguments(kj::mv(argv)));
748+
};
750749
auto timeoutId = IoContext::current().setTimeoutImpl(
751750
timeoutIdGenerator,
752751
/* repeats = */ true,
753-
[function = function.addRef(js),
754-
argv = kj::mv(argv)]
755-
(jsg::Lock& js) mutable {
756-
auto context = js.v8Context();
757-
auto localFunction = function.getHandle(js);
758-
auto localArgs = KJ_MAP(arg, argv) {
759-
return arg.getHandle(js);
760-
};
761-
auto argc = localArgs.size();
762-
763-
// Cast to void to discard the result value.
764-
(void)jsg::check(localFunction->Call(context, context->Global(), argc, &localArgs.front()));
752+
[function = kj::mv(fn)](jsg::Lock& js) mutable {
753+
function(js);
765754
}, msDelay.orDefault(0));
766755
return timeoutId.toNumber();
767756
}

‎src/workerd/api/global-scope.h

+4-4
Original file line numberDiff line numberDiff line change
@@ -327,19 +327,19 @@ class ServiceWorkerGlobalScope: public WorkerGlobalScope {
327327
jsg::Optional<StructuredCloneOptions> options);
328328

329329
TimeoutId::NumberType setTimeout(jsg::Lock& js,
330-
jsg::V8Ref<v8::Function> function,
330+
jsg::Function<void(jsg::Arguments<jsg::Value>)> function,
331331
jsg::Optional<double> msDelay,
332-
jsg::Varargs args);
332+
jsg::Arguments<jsg::Value> args);
333333
void clearTimeout(kj::Maybe<TimeoutId::NumberType> timeoutId);
334334

335335
TimeoutId::NumberType setTimeoutInternal(
336336
jsg::Function<void()> function,
337337
double msDelay);
338338

339339
TimeoutId::NumberType setInterval(jsg::Lock& js,
340-
jsg::V8Ref<v8::Function> function,
340+
jsg::Function<void(jsg::Arguments<jsg::Value>)> function,
341341
jsg::Optional<double> msDelay,
342-
jsg::Varargs args);
342+
jsg::Arguments<jsg::Value> args);
343343
void clearInterval(kj::Maybe<TimeoutId::NumberType> timeoutId) { clearTimeout(timeoutId); }
344344

345345
jsg::Promise<jsg::Ref<Response>> fetch(

‎src/workerd/api/node/async-hooks.c++

+14-32
Original file line numberDiff line numberDiff line change
@@ -13,28 +13,17 @@ jsg::Ref<AsyncLocalStorage> AsyncLocalStorage::constructor(jsg::Lock& js) {
1313
v8::Local<v8::Value> AsyncLocalStorage::run(
1414
jsg::Lock& js,
1515
v8::Local<v8::Value> store,
16-
v8::Local<v8::Function> callback,
17-
jsg::Varargs args) {
18-
kj::Vector<v8::Local<v8::Value>> argv(args.size());
19-
for (auto arg : args) {
20-
argv.add(arg.getHandle(js));
21-
}
22-
23-
auto context = js.v8Context();
24-
16+
jsg::Function<v8::Local<v8::Value>(jsg::Arguments<jsg::Value>)> callback,
17+
jsg::Arguments<jsg::Value> args) {
18+
callback.setReceiver(js.v8Ref<v8::Value>(js.v8Context()->Global()));
2519
jsg::AsyncContextFrame::StorageScope scope(js, *key, js.v8Ref(store));
26-
27-
return jsg::check(callback->Call(
28-
context,
29-
context->Global(),
30-
argv.size(),
31-
argv.begin()));
20+
return callback(js, kj::mv(args));
3221
}
3322

3423
v8::Local<v8::Value> AsyncLocalStorage::exit(
3524
jsg::Lock& js,
36-
v8::Local<v8::Function> callback,
37-
jsg::Varargs args) {
25+
jsg::Function<v8::Local<v8::Value>(jsg::Arguments<jsg::Value>)> callback,
26+
jsg::Arguments<jsg::Value> args) {
3827
// Node.js defines exit as running "a function synchronously outside of a context".
3928
// It goes on to say that the store is not accessible within the callback or the
4029
// asynchronous operations created within the callback. Any getStore() call done
@@ -45,7 +34,7 @@ v8::Local<v8::Value> AsyncLocalStorage::exit(
4534
// implementing the enterWith/disable methods. We can emulate the correct
4635
// behavior simply by calling run with the store value set to undefined, which
4736
// will propagate correctly.
48-
return run(js, js.v8Undefined(), callback, kj::mv(args));
37+
return run(js, js.v8Undefined(), kj::mv(callback), kj::mv(args));
4938
}
5039

5140
v8::Local<v8::Value> AsyncLocalStorage::getStore(jsg::Lock& js) {
@@ -127,23 +116,16 @@ v8::Local<v8::Function> AsyncResource::bind(
127116

128117
v8::Local<v8::Value> AsyncResource::runInAsyncScope(
129118
jsg::Lock& js,
130-
v8::Local<v8::Function> fn,
119+
jsg::Function<v8::Local<v8::Value>(jsg::Arguments<jsg::Value>)> fn,
131120
jsg::Optional<v8::Local<v8::Value>> thisArg,
132-
jsg::Varargs args) {
133-
kj::Vector<v8::Local<v8::Value>> argv(args.size());
134-
for (auto arg : args) {
135-
argv.add(arg.getHandle(js));
121+
jsg::Arguments<jsg::Value> args) {
122+
v8::Local<v8::Value> receiver = js.v8Context()->Global();
123+
KJ_IF_MAYBE(arg, thisArg) {
124+
receiver = *arg;
136125
}
137-
138-
auto context = js.v8Context();
139-
126+
fn.setReceiver(js.v8Ref<v8::Value>(receiver));
140127
jsg::AsyncContextFrame::Scope scope(js, getFrame());
141-
142-
return jsg::check(fn->Call(
143-
context,
144-
thisArg.orDefault(context->Global()),
145-
argv.size(),
146-
argv.begin()));
128+
return fn(js, kj::mv(args));
147129
}
148130

149131
kj::Own<jsg::AsyncContextFrame::StorageKey> AsyncLocalStorage::getKey() {

‎src/workerd/api/node/async-hooks.h

+6-6
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,12 @@ class AsyncLocalStorage final: public jsg::Object {
3737

3838
v8::Local<v8::Value> run(jsg::Lock& js,
3939
v8::Local<v8::Value> store,
40-
v8::Local<v8::Function> callback,
41-
jsg::Varargs args);
40+
jsg::Function<v8::Local<v8::Value>(jsg::Arguments<jsg::Value>)> callback,
41+
jsg::Arguments<jsg::Value> args);
4242

4343
v8::Local<v8::Value> exit(jsg::Lock& js,
44-
v8::Local<v8::Function> callback,
45-
jsg::Varargs args);
44+
jsg::Function<v8::Local<v8::Value>(jsg::Arguments<jsg::Value>)> callback,
45+
jsg::Arguments<jsg::Value> args);
4646

4747
v8::Local<v8::Value> getStore(jsg::Lock& js);
4848

@@ -173,9 +173,9 @@ class AsyncResource final: public jsg::Object {
173173

174174
v8::Local<v8::Value> runInAsyncScope(
175175
jsg::Lock& js,
176-
v8::Local<v8::Function> fn,
176+
jsg::Function<v8::Local<v8::Value>(jsg::Arguments<jsg::Value>)> fn,
177177
jsg::Optional<v8::Local<v8::Value>> thisArg,
178-
jsg::Varargs args);
178+
jsg::Arguments<jsg::Value>);
179179
// Calls the given function within this async context.
180180

181181
JSG_RESOURCE_TYPE(AsyncResource, CompatibilityFlags::Reader flags) {

‎src/workerd/api/node/diagnostics-channel.c++

+8-13
Original file line numberDiff line numberDiff line change
@@ -83,27 +83,22 @@ void Channel::unbindStore(jsg::Lock& js, jsg::Ref<AsyncLocalStorage> als) {
8383

8484
v8::Local<v8::Value> Channel::runStores(
8585
jsg::Lock& js, jsg::Value message,
86-
v8::Local<v8::Function> callback,
86+
jsg::Function<v8::Local<v8::Value>(jsg::Arguments<jsg::Value>)> callback,
8787
jsg::Optional<v8::Local<v8::Value>> maybeReceiver,
88-
jsg::Varargs args) {
88+
jsg::Arguments<jsg::Value> args) {
8989
auto storageScopes = KJ_MAP(store, stores) {
9090
return kj::heap<jsg::AsyncContextFrame::StorageScope>(js, *store.key,
9191
store.transform(js, message.addRef(js)));
9292
};
9393

9494
publish(js, message.addRef(js));
9595

96-
auto context = js.v8Context();
97-
auto receiver = maybeReceiver.orDefault([&]() -> v8::Local<v8::Value> {
98-
return context->Global();
99-
});
100-
101-
auto arguments = KJ_MAP(arg, args) -> v8::Local<v8::Value> {
102-
return arg.getHandle(js);
103-
};
104-
105-
return jsg::check(
106-
callback->Call(context, receiver, arguments.size(), arguments.begin()));
96+
v8::Local<v8::Value> receiver = js.v8Context()->Global();
97+
KJ_IF_MAYBE(val, maybeReceiver) {
98+
receiver = *val;
99+
}
100+
callback.setReceiver(js.v8Ref(receiver));
101+
return callback(js, kj::mv(args));
107102
}
108103

109104
void Channel::visitForGc(jsg::GcVisitor& visitor) {

‎src/workerd/api/node/diagnostics-channel.h

+6-4
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,12 @@ class Channel : public jsg::Object {
2323
void bindStore(jsg::Lock& js, jsg::Ref<AsyncLocalStorage> als,
2424
jsg::Optional<TransformCallback> maybeTransform);
2525
void unbindStore(jsg::Lock& js, jsg::Ref<AsyncLocalStorage> als);
26-
v8::Local<v8::Value> runStores(jsg::Lock& js, jsg::Value message,
27-
v8::Local<v8::Function> callback,
28-
jsg::Optional<v8::Local<v8::Value>> maybeReceiver,
29-
jsg::Varargs args);
26+
v8::Local<v8::Value> runStores(
27+
jsg::Lock& js,
28+
jsg::Value message,
29+
jsg::Function<v8::Local<v8::Value>(jsg::Arguments<jsg::Value>)> callback,
30+
jsg::Optional<v8::Local<v8::Value>> maybeReceiver,
31+
jsg::Arguments<jsg::Value> args);
3032

3133
JSG_RESOURCE_TYPE(Channel) {
3234
JSG_METHOD(hasSubscribers);

‎src/workerd/jsg/function.h

+49
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include <kj/function.h>
1111
#include "wrappable.h"
1212
#include "meta.h"
13+
#include "jsg.h"
1314

1415
namespace workerd::jsg {
1516

@@ -410,6 +411,54 @@ class FunctionWrapper {
410411
V8Ref(isolate, parentObject.orDefault(context->Global())),
411412
V8Ref(isolate, handle.As<v8::Function>()));
412413
}
414+
415+
template <typename Ret>
416+
kj::Maybe<Function<Ret(Arguments<Value>)>> tryUnwrap(
417+
v8::Local<v8::Context> context,
418+
v8::Local<v8::Value> handle,
419+
Function<Ret(Arguments<Value>)>*,
420+
kj::Maybe<v8::Local<v8::Object>> parentObject) {
421+
if (!handle->IsFunction()) {
422+
return nullptr;
423+
}
424+
425+
auto isolate = context->GetIsolate();
426+
427+
auto wrapperFn = [](Lock& js,
428+
v8::Local<v8::Value> receiver,
429+
v8::Local<v8::Function> func,
430+
Arguments<Value> args) -> Ret {
431+
auto isolate = js.v8Isolate;
432+
auto& typeWrapper = TypeWrapper::from(isolate);
433+
434+
return js.withinHandleScope([&] {
435+
auto context = js.v8Context();
436+
437+
v8::Local<v8::Value> result;
438+
if (args.size() > 0) {
439+
KJ_STACK_ARRAY(v8::Local<v8::Value>, argv, args.size(), 20, 20);
440+
for (size_t n = 0; n < args.size(); n++) {
441+
argv[n] = args[n].getHandle(js);
442+
}
443+
result = check(func->Call(context, receiver, argv.size(), argv.begin()));
444+
} else {
445+
result = check(func->Call(context, receiver, 0, nullptr));
446+
}
447+
448+
if constexpr(!isVoid<Ret>()) {
449+
return typeWrapper.template unwrap<Ret>(
450+
context, result, TypeErrorContext::callbackReturn());
451+
} else {
452+
return;
453+
}
454+
});
455+
};
456+
457+
return Function<Ret(Arguments<Value>)>(
458+
wrapperFn,
459+
V8Ref(isolate, parentObject.orDefault(context->Global())),
460+
V8Ref(isolate, handle.As<v8::Function>()));
461+
}
413462
};
414463

415464
template <typename Func>

‎src/workerd/jsg/type-wrapper.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -510,7 +510,8 @@ class TypeWrapper: public DynamicResourceTypeMap<Self>,
510510
return Varargs(parameterIndex, args);
511511
} else if constexpr(isArguments<V>()) {
512512
using E = typename V::ElementType;
513-
auto builder = kj::heapArrayBuilder<E>(args.Length() - parameterIndex);
513+
size_t size = args.Length() >= parameterIndex ? args.Length() - parameterIndex : 0;
514+
auto builder = kj::heapArrayBuilder<E>(size);
514515
for (size_t i = parameterIndex; i < args.Length(); i++) {
515516
builder.add(unwrap<E>(context, args[i], errorContext));
516517
}

0 commit comments

Comments
 (0)