feat(core): Add ability to "ref" and "unref" pending ops #12889

bartlomieju · 2021-11-24T16:29:29Z

This commit adds an ability to "ref" or "unref" pending ops.

Up to this point Deno had a notion of "async ops" and "unref async ops";
the former keep event loop alive, while the latter do not block event loop
from finishing. It was not possible to change between op types after
dispatching, one had to decide which type to use before dispatch.

Instead of storing ops in two separate "FuturesUnordered" collections,
now ops are stored in a single collection, with supplemental "HashSet"
storing ids of promises that were "unrefed".

Two APIs were added to "Deno.core":

"Deno.core.refOp(promiseId)" which allows to mark promise id
to be "refed" and keep event loop alive (the default behavior)
"Deno.core.unrefOp(promiseId)" which allows to mark promise
id as "unrefed" which won't block event loop from exiting

Closes #12745

TODO:

metrics for unrefed ops (I think that unrefing ops should decrement ops_dispatched_async and increment ops_dispatched_async_unref, though it seems overly complicated)
cleanup handling of finished ops (remove AsyncOpIterator?)
add id field to promises returned from Deno.core.opcallAsync
add Deno.core.ref(...promiseIds) and Deno.core.unref(...promiseIds)

bartlomieju · 2021-11-24T16:30:05Z

core/01_core.js

+ function opAsync(opName, arg1 = null, arg2 = null, unref = false) {
 const promiseId = nextPromiseId++;
- const maybeError = opcallAsync(opsCache[opName], promiseId, arg1, arg2);
+ const maybeError = opcallAsync(
+ opsCache[opName],
+ promiseId,
+ arg1,
+ arg2,
+ unref,
+ );


This change was done so that ops that are unref by default don't have to call into Rust immediately after dispatch to mark them as "unrefed"

I probably would keep the old signature. The only "unref-by-default" ops are signal listeners and they're not a performance concern.

core/runtime.rs

piscisaureus · 2021-11-24T18:00:57Z

core/01_core.js

- return PromisePrototypeThen(setPromise(promiseId), unwrapOpResult);
+ const p = PromisePrototypeThen(setPromise(promiseId), unwrapOpResult);
+ // Save the id on the promise so it can be latered ref'ed or unref'ed
+ p.id = promiseId;


Have you considered using a symbol here or a reverse WeakMap?
(I am not sure what that would do for performance, we'd have to measure that)

I did not, but using a symbol might be preferable, that way we can better communicate that users shouldn't use that value. I'll explore that.

Used Symbol.for("Deno.core.internalPromiseId") instead, let's see if performance is better or worse

piscisaureus · 2021-11-24T18:03:10Z

core/lib.deno_core.d.ts

+ /** Mark following promises as "ref", ie. event loop won't exit
+ * until they are resolved. All async ops are "ref" by default. */
+ function refOps(
+ ...promiseIds: number[]


Why does it take multiple promiseIds?
I do understand that it is faster to coalesce multiple refOp calls into a single call, but I wonder how common that is, and also note that passing a single int into the binding layer is faster than passing an array.

In some resources we might have multiple pending ops associated with it. So instead of crossing boundary multiple times I thought it's better to do it in one go. Keep in mind that this is spread operator so you're passing multiple integers and not an array (Deno.core.refOps(1, 2, 3, 4)). Also is there a situation where this API would be used in a hot path?

Keep in mind that this is spread operator so you're passing multiple integers and not an array (Deno.core.refOps(1, 2, 3, 4))

I see, in the binding layer you're just iterating over FunctionCallbackArguments, that's pretty efficient and I have no concerns about it.

In some resources we might have multiple pending ops associated with it.

You'll find that this almost never happens in practice but oh well.

If you prefer to have it accept single ID that is fine for me as well. Your call

core/runtime.rs

piscisaureus

Looks pretty good, some questions though.

AaronO · 2021-11-24T18:06:10Z

Do we want all op(calls) to be unrefable ? I feel like it should be opt-in and take its distinct path (e.g: opUnref instead of overloading opAsync)

piscisaureus · 2021-11-24T18:08:14Z

Do we want all op(calls) to be unrefable ? I feel like it should be opt-in and take its distinct path (e.g: opUnref instead of overloading opAsync)

I don't see a reason why we would be more restrictive about this than necessary.

AaronO · 2021-11-24T18:12:52Z

Do we want all op(calls) to be unrefable ? I feel like it should be opt-in and take its distinct path (e.g: opUnref instead of overloading opAsync)

I don't see a reason why we would be more restrictive about this than necessary.

Well if in practice there's only a very small subset of ops/opcalls that may be unref'd then I think it's cleaner to have its own distinct implementation path so as not to overload the simple async opcall path

bartlomieju · 2021-11-24T18:21:29Z

Do we want all op(calls) to be unrefable ? I feel like it should be opt-in and take its distinct path (e.g: opUnref instead of overloading opAsync)

I don't see a reason why we would be more restrictive about this than necessary.

Well if in practice there's only a very small subset of ops/opcalls that may be unref'd then I think it's cleaner to have its own distinct implementation path so as not to overload the simple async opcall path

I agree with Bert, I also believe it makes it more confusing to have distinct type of op for that purpose. Besides having different API there would be no upside IMO as we won't be able to validate if you are unrefing an op of that type. I'll be happy to revisit the topic after we add .ref()/.unref() to several APIs.

AaronO · 2021-11-24T18:32:50Z

Do we want all op(calls) to be unrefable ? I feel like it should be opt-in and take its distinct path (e.g: opUnref instead of overloading opAsync)

I don't see a reason why we would be more restrictive about this than necessary.

Well if in practice there's only a very small subset of ops/opcalls that may be unref'd then I think it's cleaner to have its own distinct implementation path so as not to overload the simple async opcall path

I agree with Bert, I also believe it makes it more confusing to have distinct type of op for that purpose. Besides having different API there would be no upside IMO as we won't be able to validate if you are unrefing an op of that type. I'll be happy to revisit the topic after we add .ref()/.unref() to several APIs.

I'm not proposing a different kind of op rust-side just that the opCall JS side is explicit (killing op_async_unref is good IMO).

Do we want all op(calls) to be unrefable ? I feel like it should be opt-in and take its distinct path (e.g: opUnref instead of overloading opAsync)

I don't see a reason why we would be more restrictive about this than necessary.

It all depends on our call expectations but IMO:

Deno.core.opAsync("op_foo", A, B, unref=true/false) makes most sense if unref is derived from end-user args/inputs
Deno.core.opUnref("op_foo", A, B) makes more sense if the implementor decides if the opcall is unrefable

From my understanding unref=... shouldn't really be derived from user inputs, it will mostly be hardcoded by the implementor (ext/*/*.js code)

If that's the case I think Deno.core.opUnref is the better choice, it explicits the intention and is easier to search/scan.

I also think it's more ergonomic, the previous opAsync() sig maps quite naturally to the function sig, one could view it as a curried version of the opcall or Deno.core.ops.foo_bar(A, B) (which would be the API if we did Fast API), tagging an op-layer control value to the end of the args feels out of place

bartlomieju · 2021-11-24T18:34:28Z

I'm not proposing a different kind of op rust-side just that the opCall JS side is explicit (killing op_async_unref is good IMO).

Okay, I misunderstood then. @AaronO I removed unref argument in Deno.core.opAsync; now you need to manually call Deno.core.unrefOps() afterwards. Is that okay for you?

bartlomieju · 2021-11-24T18:35:20Z

Also I'd appreciate some feedback on how to tackle metrics - right now there are two uncalled APIs, this needs to be addressed before landing.

piscisaureus · 2021-11-24T18:38:13Z

core/ops_metrics.rs

@@ -85,12 +85,14 @@ impl OpsTracker {
 metrics.ops_completed_async += 1;
 }

+ #[allow(unused)]
 pub fn track_unref(&self, id: OpId) {


Just delete this, I guess?
@AaronO any opinions?

I guess that's fine. Right now "unrefed" ops don't take part in opSanitizer but actually it might be preferable to include them.

core/01_core.js

piscisaureus

LGTM, let's land this and see what it does to the benchmarks.

AaronO

LGTM

bnoordhuis

Nice work!

bnoordhuis · 2021-11-25T08:07:25Z

core/01_core.js

@@ -43,6 +44,7 @@
 const RING_SIZE = 4 * 1024;
 const NO_PROMISE = null; // Alias to null is faster than plain nulls
 const promiseRing = ArrayPrototypeFill(new Array(RING_SIZE), NO_PROMISE);
+ const promiseIdSymbol = SymbolFor("Deno.core.internalPromiseId");


Non-blocking but as a possible future improvement: you can use a v8::Private if you're worried about the symbol leaking out to user land code (it's visible to Object.getOwnPropertySymbols() after all.)

Private symbols are like regular symbols except more... er, private. (Great explanation, Ben. +1, would read again.)

Is that construct available in JS land?

Not yet, but I can add it. You may have seen node's getHiddenValue() and setHiddenValue() - it's the same mechanism.

Not yet, but I can add it. You may have seen node's getHiddenValue() and setHiddenValue() - it's the same mechanism.

I haven't seen it. I guess I'll add a TODO and we can change it once the API is added.

core/bindings.rs

core/ops_metrics.rs

core/runtime.rs

core/01_core.js

andreubotella · 2021-11-25T14:41:25Z

core/bindings.rs

+ }
+ };
+
+ state.unrefed_ops.insert(promise_id);


Can we check that promise_id is in state.pending_ops?

No, we'd have to iterate over all pending_ops killing the performance.

Oh wait, this is bindings.rs, it's not a hot path. I guess I can add a check here.

Actually not possible to do that, there's no way to inspect promise_id when iterating pending_ops

This might be possible once @AaronO moves promise ring to Rust (#12314)

In the meantime, what probably can be done is change the unrefOp JS function to check whether the promise ID exists and hasn't resolved, right?

It would be pretty bad if some careless code added an already-resolved promise ID to unrefed_ops, since all that matters is the length compared to pending_ops. But I suppose CI would catch that, though.

In the meantime, what probably can be done is change the unrefOp JS function to check whether the promise ID exists and hasn't resolved, right?

That would require to adding a wrapper around Deno.core.unrefOp in core/01_core.js just to verify the promise map.

I'm not sure if it's worth doing that at this moment; I think we should try this API in the wild and see if that's a problem.

bartlomieju added 3 commits November 24, 2021 17:14

refactor(core): handling of unref ops

4562596

migrate signals to unref ops

02563dc

remove op_async_unref

b064c26

bartlomieju commented Nov 24, 2021

View reviewed changes

bartlomieju requested review from AaronO, bnoordhuis and piscisaureus November 24, 2021 16:30

bartlomieju added 4 commits November 24, 2021 17:37

remove AsyncOpIterator

302ccfd

lint

a0f3f6b

add id to promises returned from Deno.core.opcallAsync

96b3b8d

Add Deno.core.refOps() and Deno.core.unrefOps()

f1baa37

bartlomieju changed the title ~~[WIP] refactor(core): handling of unref ops~~ refactor(core): handling of unref ops Nov 24, 2021

bartlomieju changed the title ~~refactor(core): handling of unref ops~~ feat(core): Add ability to "ref" and "unref" pending ops Nov 24, 2021

piscisaureus reviewed Nov 24, 2021

View reviewed changes

core/runtime.rs Outdated Show resolved Hide resolved

piscisaureus reviewed Nov 24, 2021

View reviewed changes

Bert's review: revert opAsync sig, use symbol for storing promise id

c9de904

piscisaureus reviewed Nov 24, 2021

View reviewed changes

core/01_core.js Outdated Show resolved Hide resolved

piscisaureus approved these changes Nov 24, 2021

View reviewed changes

revert unnecessary change

77d0253

AaronO approved these changes Nov 24, 2021

View reviewed changes

bartlomieju added 7 commits November 24, 2021 20:31

remove ops_metrics methods

c3b7e4d

reset CI

947d498

replace Deno.core.refOps with refOp and unrefOps with unrefOp

88d9b86

hoist symbol in tests

83be1c0

Merge branch 'main' into unref_ops

7b19786

reset CI

b599e5a

reset CI2

abb64a5

bnoordhuis approved these changes Nov 25, 2021

View reviewed changes

bartlomieju added 2 commits November 25, 2021 14:21

Ben's review

5344c16

reset CI

c6fd22b

andreubotella reviewed Nov 25, 2021

View reviewed changes

bartlomieju merged commit f3c0f05 into denoland:main Nov 25, 2021

bartlomieju deleted the unref_ops branch November 25, 2021 18:49

kt3k mentioned this pull request Mar 11, 2022

refactor(core): validate promise id in refOp #13905

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): Add ability to "ref" and "unref" pending ops #12889

feat(core): Add ability to "ref" and "unref" pending ops #12889

bartlomieju commented Nov 24, 2021 •

edited

Loading

bartlomieju Nov 24, 2021

piscisaureus Nov 24, 2021

bartlomieju Nov 24, 2021

piscisaureus Nov 24, 2021

bartlomieju Nov 24, 2021

bartlomieju Nov 24, 2021

piscisaureus Nov 24, 2021

bartlomieju Nov 24, 2021

piscisaureus Nov 24, 2021

bartlomieju Nov 24, 2021

piscisaureus left a comment

AaronO commented Nov 24, 2021

piscisaureus commented Nov 24, 2021

AaronO commented Nov 24, 2021

bartlomieju commented Nov 24, 2021

AaronO commented Nov 24, 2021 •

edited

Loading

bartlomieju commented Nov 24, 2021

bartlomieju commented Nov 24, 2021

piscisaureus Nov 24, 2021

bartlomieju Nov 24, 2021

piscisaureus left a comment

AaronO left a comment

bnoordhuis left a comment

bnoordhuis Nov 25, 2021

bartlomieju Nov 25, 2021

bnoordhuis Nov 25, 2021

bartlomieju Nov 25, 2021

andreubotella Nov 25, 2021

bartlomieju Nov 25, 2021

bartlomieju Nov 25, 2021

bartlomieju Nov 25, 2021

bartlomieju Nov 25, 2021

andreubotella Nov 25, 2021

bartlomieju Nov 25, 2021

feat(core): Add ability to "ref" and "unref" pending ops #12889

feat(core): Add ability to "ref" and "unref" pending ops #12889

Conversation

bartlomieju commented Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piscisaureus left a comment

Choose a reason for hiding this comment

AaronO commented Nov 24, 2021

piscisaureus commented Nov 24, 2021

AaronO commented Nov 24, 2021

bartlomieju commented Nov 24, 2021

AaronO commented Nov 24, 2021 • edited Loading

bartlomieju commented Nov 24, 2021

bartlomieju commented Nov 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piscisaureus left a comment

Choose a reason for hiding this comment

AaronO left a comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju commented Nov 24, 2021 •

edited

Loading

AaronO commented Nov 24, 2021 •

edited

Loading