Prevent interruptAndCheck from delaying too often when delay takes to… #1787
Conversation
…o long
I ran into an issue where Firefox setTimeout(resolve, 0) was taking the majority of the time between interruptAndCheck calls.
performance.now() - current will be greater than globalInterruptionPeriod so it gets called every iteration
As a work around I can greatly increase the globalinterruptionPeriod with something like setInterruptionPeriod(150) - but then other browsers that don't have the slower setTimeout callback will have slower cancellation responses.
Other fixes I considered: have delayNextTick return performance.now() only for setTimout path (code below) or letting users provide a function that gets called back with current and they can either return current or process.now()
export function delayNextTick(current: number): Promise<number> {
return new Promise(resolve => {
// In case we are running in a non-node environment, `setImmediate` isn't available.
// Using `setTimeout` of the browser API accomplishes the same result.
if (typeof setImmediate === 'undefined') {
// there is no way to guarantee
setTimeout(() => resolve(performance.now()), 0);
} else {
setImmediate(() => resolve(current));
}
});
}
There was a problem hiding this comment.
@thejoecode Quick question before going deeper into this: When using Langium in a browser, do you run it in a dedicated worker? I'm a bit confused on why FireFox is even interrupting the event loop that long. I'm driving FireFox as well and never had any issues with Langium in the browser yet.
I am using a Worker like so and passing it as part of the user config in MonacoEditorLanguageClientWrapper: The first loop I bottle neck in is: DefaultScopeComputation->computeExportsForNode If I override that function and remove the interruptAndCheck() it just moves the issue to computeLocalScopes() and after that to another. I stopped overriding and dug into why Chrome didn't need the overrides but Firefox did. My best guess is that Firefox spends more time restoring the context after a setTimeout call. |
Putting initial set of lastTick to current back from feedback @msujew wrote: If we interrupt to let other code run (i.e. how it's intended to be used, not just waiting on FireFox to do it's thing), the newly called code will then also call interruptAndCheck which immediately results in another next tick delay. We don't want that, so we would need to update the lastTick before and after running the delay.
There was a problem hiding this comment.
I believe that's a good change. @spoenemann what do you think?
|
I was actually able to reproduce this in a project and it essentially makes the language server unusable. I'll provide a patch release soon that includes this change once we merge this. This honestly seems like a FireFox bug, but I think it makes sense anyway to include this fix anyway. |
…o long
I ran into an issue where Firefox setTimeout(resolve, 0) was taking the majority of the time between interruptAndCheck calls.
performance.now() - current will be greater than globalInterruptionPeriod so it gets called every iteration
As a work around I can greatly increase the globalinterruptionPeriod with something like setInterruptionPeriod(150) - but then other browsers that don't have the slower setTimeout callback will have slower cancellation responses.
Other fixes I considered: have delayNextTick return performance.now() only for setTimout path (code below) or letting users provide a function that gets called back with current and they can either return current or process.now()