Skip to content

Conversation

@Tyriar
Copy link
Member

@Tyriar Tyriar commented Oct 11, 2022

Part of #161622

@Tyriar Tyriar added this to the October 2022 milestone Oct 11, 2022
@Tyriar Tyriar requested a review from jrieken October 11, 2022 15:17
@Tyriar Tyriar self-assigned this Oct 11, 2022
@Tyriar Tyriar marked this pull request as draft October 11, 2022 22:56
@Tyriar Tyriar marked this pull request as ready for review October 13, 2022 19:54
@Tyriar Tyriar marked this pull request as draft October 13, 2022 19:54
@Tyriar
Copy link
Member Author

Tyriar commented Oct 14, 2022

I found that about 20-30% of keypress time was spend setting up or tearing down timeouts/callback. This change introduces Event.defer which now is now a convenience function on top of Event.debounce which converts it to a debounced signal. It converts some low-risk listeners which simply schedule things to a following task to use deferred listeners to avoid creating a new timeout for each one.

Before:

image

The above was taken using the current change but swapping out defer's implementation with signal(event) to get an approximation of what it was before.

After:

image

This was tested on my fast machine so the numbers are pretty small, they get quite a bit larger on slower hardware. Async stack traces were disabled to get more accurate timeout measurements.

@Tyriar Tyriar requested review from alexdima and hediet October 14, 2022 12:14
@Tyriar Tyriar marked this pull request as ready for review October 14, 2022 12:14
Copy link
Member

@jrieken jrieken left a comment

Choose a reason for hiding this comment

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

Thanks for kicking this off. It's an interesting approach to debounce at the emitter and not at listeners. I believe either works and I let @alexdima and @hediet decide what they prefer. I do see the advantage of such an offering - it is a nice and central invitation/reminder to be a nice listener

The disposable store for the defer util is important and shouldn't be forgotten

* block critical work (eg. latency of keypress to text rendered).
*/
export function defer(event: Event<unknown>): Event<void> {
return debounce<unknown, void>(event, () => void 0, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Needs to have the ability to be created with a disposable store (see other utils and doc) because otherwise a leaked listener keeps this alive

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 will keep in mind for next iteration


private readonly _onDidChangeModelContent: Emitter<IModelContentChangedEvent> = this._register(new Emitter<IModelContentChangedEvent>({ deliveryQueue: this._deliveryQueue }));
public readonly onDidChangeModelContent: Event<IModelContentChangedEvent> = this._onDidChangeModelContent.event;
public readonly onDidChangeModelContentDeferred: Event<void> = Event.defer(this._onDidChangeModelContent.event);
Copy link
Member

Choose a reason for hiding this comment

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

Needs to pass this._store to dispose this event when the editor is dispsoed


// feature: update active when cursor changes
this._outlineDisposables.add(this._editor.onDidChangeCursorPosition(_ => {
this._outlineDisposables.add(this._editor.onDidChangeCursorPositionDeferred(_ => {
Copy link
Member

Choose a reason for hiding this comment

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

This listener is already async (TimeoutTimer#cancelAndSet) and I don't think double debounce is needed

Copy link
Member Author

Choose a reason for hiding this comment

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

This was what I was trying to avoid, deferring the extra clear and timeout calls into the existing timeout, but it didn't give the gains expected.

editor?.onDidChangeModelLanguage(() => this._update(true), this, this._renderDisposables);
editor?.onDidChangeModelContent(() => this._update(false), this, this._renderDisposables);
editor?.onDidChangeModelLanguage(() => this._updateLanguage(true), this, this._renderDisposables);
editor?.onDidChangeModelContentDeferred(() => this._scheduleUpdate(), this, this._renderDisposables);
Copy link
Member

Choose a reason for hiding this comment

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

again double debounce?

@Tyriar
Copy link
Member Author

Tyriar commented Oct 14, 2022

Making this a draft again, still measuring differences

@Tyriar Tyriar marked this pull request as draft October 14, 2022 13:14
@Tyriar
Copy link
Member Author

Tyriar commented Oct 14, 2022

Some measurements via performance.now() (0.1ms precision):

Typing 'a' on my macbook until line is full (to avoid scrolling overhead), ignore first 10 samples as suggest is involved

PR where defer uses debounce

averageWithoutFirst 2.341666666428662 sample size 144
averageWithoutFirst 2.2861111108731063 sample size 144
averageWithoutFirst 2.3076388886612325 sample size 144
averageWithoutFirst 2.353472222470575 sample size 144

PR where defer uses signal

averageWithoutFirst 2.33819444430992 sample size 144
averageWithoutFirst 2.392361111162851 sample size 144
averageWithoutFirst 2.3055555556072957 sample size 144
averageWithoutFirst 2.3951388884128795 sample size 144

main

averageWithoutFirst 2.3451388888578446 sample size 144
averageWithoutFirst 2.400694444672101 sample size 144
averageWithoutFirst 2.347916666366574 sample size 144
averageWithoutFirst 2.1895833337265582 sample size 144
averageWithoutFirst 2.425000000020696 sample size 144

These results show that it may be slightly faster, but not as much as I was expecting.

I notice the JavaScript Profiler is far more accurate than the Performance tab in devtools for measuring this low level stuff so I should probably be using that to analyze. It also seems to avoid the timeout overhead problem. Closing this for now, will investigate further.

@Tyriar Tyriar closed this Oct 14, 2022
@jrieken
Copy link
Member

jrieken commented Oct 14, 2022

I notice the JavaScript Profiler is far more accurate than the Performance tab in devtools for measuring this low level stuff

Yeah, that matches my experience. Tho, it lacks the holistic view of things

@Tyriar
Copy link
Member Author

Tyriar commented Oct 14, 2022

Did more thorough performance testing using a JS CPU profile, couldn't see a particular difference with this change of tweaking how scheduling things was done.

Details Some more measurements, this time using JavaScript Profiler on a profile with the following test:
  • Make InputHandler.ts (xterm.js) dirty
  • Reload
  • Wait for renderer to calm down
  • Type a 150 times at the bottom of _preserveStack

Before - main

Profile:

codeEditorWidget.ts:1616 = 9.37% of keypress fn

Focusing on that function:

Screen Shot 2022-10-14 at 10 08 55 am

After - Event.defer

codeEditorWidget.ts:1619 = 9.36% of keypress fn

Focusing on that function:

Screen Shot 2022-10-14 at 10 16 48 am

@github-actions github-actions bot locked and limited conversation to collaborators Nov 28, 2022
@Tyriar Tyriar deleted the tyriar/161622_event_defer branch December 15, 2022 18:39
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.

3 participants