Skip to content

add pump message loop calls #829

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

Merged
merged 8 commits into from
Jul 3, 2025
Merged

add pump message loop calls #829

merged 8 commits into from
Jul 3, 2025

Conversation

krichprollsch
Copy link
Member

@krichprollsch krichprollsch commented Jun 30, 2025

@krichprollsch krichprollsch self-assigned this Jun 30, 2025
@krichprollsch krichprollsch changed the title add pump message loop calls WIP add pump message loop calls Jul 1, 2025
@krichprollsch krichprollsch changed the title WIP add pump message loop calls add pump message loop calls Jul 1, 2025
@@ -260,6 +263,24 @@ pub fn Env(comptime State: type, comptime WebApis: type) type {
self.isolate.performMicrotasksCheckpoint();
}

pub fn pumpMessageLoop(self: *const Self) bool {
if (self.platform == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since these are potentially called in a tight loop, I think you can do this at comptime:

if (comptime builtin.is_test) {
    if (self.platform == null) return false;
}
// assume it's not-null in non-test.
return self.platform.?.inner.pumpMessageLoop(self.isolate, false);

Copy link
Member Author

Choose a reason for hiding this comment

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

I was wondering if I should use a different slower loop for these tasks separated from the microtasks one.
WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand what this is doing, so it's hard to say.

The page's messageLoopCallback is not being called very often. all of page.navigate (including processing all synchronous scripts) are executed before it's ever called. This could change if navigate became fully asynchronous though.

It's only being called often when we're "done" navigating and waiting for
1 - New cdp sessions
2 - I/O (i.e. an XHR request is live)

In both cases, I would think it's mostly a no-op.

@karlseguin
Copy link
Collaborator

Besides the small [optional] optimization tweak above, PR is good.

Probably some opportunities to tighten up the test code. There's always a platform, but the fact that you don't have access to it in some parts of the test code is unfortunate. I think it's just that serveCDP that's the issue. Maybe we don't need those tests and instead rely on the integration / demo.

@krichprollsch
Copy link
Member Author

pump message loop could be relative to v8's garbage collector, but I'm not sure about that.
I suppose it's useful to run it time to time.
here we run it 1ms after page's initialize and then every 100ms. I think it's a good enough balance to start.

idk for idleTasks...

@krichprollsch krichprollsch merged commit f51ee7f into main Jul 3, 2025
10 checks passed
@krichprollsch krichprollsch deleted the pumpmessageloop branch July 3, 2025 17:11
@github-actions github-actions bot locked and limited conversation to collaborators Jul 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants