Skip to content
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

[Performance] Lots of timers installed and cleared #765

Closed
josephliccini opened this issue Dec 11, 2019 · 4 comments
Closed

[Performance] Lots of timers installed and cleared #765

josephliccini opened this issue Dec 11, 2019 · 4 comments
Labels
area: runtime Runtime related issues perf

Comments

@josephliccini
Copy link

Hi,

I have a SessionID: ca3082a6-2a97-4699-9add-eda17ad3b702
And a profile that goes with it: https://microsoft-my.sharepoint-df.com/:u:/p/joliccin/EX1bkObepNRKkRquZ9DsoCgBwUSiPAxCXkwSwlA75IDWeA?e=5TsSYQ

In the profile, I'm interested in the 9000ms to 10000ms part (not the Scriptor stuff before that, I have already talked to Frankie about that)

I am seeing that Fluid is running some heursitic that says "Should I take a snapshot?", and it seems to be installing and clearing timers frequently. Since this is a browser API, it's not the most efficient thing to do in high frequency.

This is the function, in officeFluidContainer.js

handleOp(e, t) {
    void 0 === e && (this.heuristics.lastOpSeqNumber = t.sequenceNumber,
    t.type === d.a.Save ? this.trySummarize(`;${t.clientId}: ${t.contents}`) : this.heuristics.run())
}

Here's what it looks like zoomed in:
image

a little zoomed out:
image

a little more:
image

It's not the biggest bottleneck here in isolation, but if you don't need to install as many timers, you may be able to save a good amount of time in aggregate, especially on slower devices.

@tylerbutler
Copy link
Member

I think the source function is in packages\runtime\container-runtime\src\summarizer.ts, the handleOp method. @arinwt @vladsud - FYI

@vladsud
Copy link
Contributor

vladsud commented Dec 12, 2019

Thanks for bringing it up! Yes, I think we need to address it.
I have faced similar problem in native world (where rich office client attempt to measure responsiveness through setting and resetting timer on each message) was killing scalability of office on terminal server.
We addressed it twofold - as an optimization in Windows itself, but also in Office through an abstraction over timers that was not cancelling timer (and reusing it later) right away if if was too far out and the cost of firing unneeded timer (battery, scalability) was not high enough compared to potential gain of not continuously cancelling and rescheduling a timer.

@curtisman curtisman added the area: runtime Runtime related issues label Jan 31, 2020
@curtisman curtisman added this to the Future milestone Feb 2, 2020
@curtisman curtisman modified the milestones: Future, Next 2020 Feb 11, 2020
@vladsud
Copy link
Contributor

vladsud commented May 28, 2021

@arinwt, is this already addressed? That's an old bug, and I believe we no longer have issues with timers here, but worth confirming.

@arinwt
Copy link
Contributor

arinwt commented May 28, 2021

I did address the excessive restarts within summarizer code using restart() in #770, so yes I think we can close this. There may be other places in our code that aren't using the optimized restart function, but it would only matter if they are excessive.

@arinwt arinwt closed this as completed May 28, 2021
@danielroney danielroney removed this from the Next milestone Jun 3, 2021
jason-ha pushed a commit that referenced this issue Jan 23, 2025
* version update for brainstorm and collaborative text area (#764)

* version update for brainstorm and collaborative text area

* remove azure-client from collaborative-text-area example, update brainstorm dependencies to use internal instead of dev

* Node demo and dice roller demo update to tinylicious 2.0 itnernal (#765)

* update dice roller to 2.0-internal tinylicious

* update node demo to tinylicious 2.0 internal

* V2.0 internal updates (#766)

* react-demo and react-starter-template

* audience demo and angular demo

* update FluidExamples 2.0.0-internal to latest internal version (#783)

* V2.0 internal update (#792)

* remove teams-fluid-hello-world example (#767)

* build: Misc. cleanup (#768)

* docs: Misc. README cleanup

* build: Update lockfile

* build: Update lockfile

* docs: Misc README cleanup

* docs: Update READMEs

* docs: Update README

* build: Update lockfile

* docs: Update README

* improvement: useMemo

* docs: Update README

* style: Sort package.json

* build: Update lockfile

* refactor: Update start flow to match other packages

* build: Update lockfile

* docs: Fix typo

* build: Update start flow to match other packages

* docs: Update README

* ci: Remove obsolete template

* fix: config

* style: Prettier

* style: Sort scripts

* style: Sort scripts

* revert: lockfile update

Seems to have broken tests

* brainstorm demo fix for remote azure example (#769)

* Add schedule to build apps daily (#771)

Add a schedule to automatically run builds on a daily basis

AB#4694

* docs: Clarify app usage instructions (#774)

Update app running instructions to be more consistent and explicit.

* tools: Update test infra to generate test reports locally and in CI (#775)

CI was set up to publish generated test reports, but none were being generated. This PR updates our testing logic to ensure test reports are correctly generated by test runs.

E.g.
![image](https://github.com/microsoft/FluidExamples/assets/54606601/4642657e-6e69-4f10-bba2-10ef5bd80a9c)

* Increase timeout for react-starter-template tests (#776)

This PR increases the timeout for react-starter-template tests, since they were consistently timing out in automated test pipelines.

* Bump @types/node to v16 (#777)

* Increase Timeouts (#780)

* Update root deps (#778)

Make syncpack a dev dep, update it and regenerate lock file.

* Update audience-demo (#782)

Update deps and improve readme for audience-demo.

Also updates CI from node 14 to node 16 to support ??= syntax.

These updates might help with the node update (#779 ), but with the unrelated non-deterministic CI errors and no useful error output, that's just a guess.

* Misc Updates and cleanup (#785)

Pull in updates from 61415de except for updating the node version, then npm install in all the examples.

This mainly updates a lot of deps to help enable node 18, and fixes many minor issues (updates to address deprecation warning from the new versions, fix alphabetical ordering, mode dev deps to dev deps etc.)

* Revert "Misc Updates and cleanup (#785)" (#786)

This reverts commit 11030fb.

* Un-revert dependency updates (#787)

Updates from #785, except for the build.yml change (which broke test running), and adding update to one jest version which caused a failing test.

* Update build.yml (#788)

Remaining changes from #785, cleaning up the build.yml. Includes a fix from that version which fixes test running.

This does not include the Node version update: that will follow in a separate change.

* Use node 18 on CI (#779)

Update to a supported version of NodeJs for CI.

* Use node 20 (#789)

Use and recommend (via nvm) Node 20.
New projects starting now should probably use Node 20, since it will be active LST and thus the best choice for production in just over a month.

Also, the Fluid project, as library authors, is granted the window of a Node release being "Current" (before it goes LTS) to ensure our libraries work: running these examples on the "current" release, Node 20, can help with that.

Support for node 18 will be kept (in engines and @types/node) for those who wish to use it, but it will not be tested by CI.

* Only support Node 18 and newer (#790)

Require at least Node 18.

This removes support for older unsupported major versions of NodeJs.

* version bump for brainstorm and collaborative text-area

* version bump for dice roller and audience demo

* version bump for react and node demo

* version bump react starter template

* angular demo bump

---------

Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>
Co-authored-by: Scott Norton <scottnorton@microsoft.com>
Co-authored-by: Sonali Deshpande <48232592+sonalideshpandemsft@users.noreply.github.com>
Co-authored-by: Craig Macomber (Microsoft) <42876482+CraigMacomber@users.noreply.github.com>

* Add v2.0_internal to yml file (#795)

* Bump to internal 7.1.0 (#796)

Also switches to test-runtime-utils in place of test-client-utils and bumps build-common to 2.0.2.

* bump to 2.0.0-internal.7.3.0 (#798)

Bump internal branch 2.0 packages to 2.0.0-internal.7.2.0

* bump package dependencies to 2.0.0-internal.7.2.2 (#800)

Co-authored-by: Michael Zhen <michaelzhen@WIN-1V7LUTHS292.redmond.corp.microsoft.com>

* bump FF package dependencies to 2.0.0-internal.7.3.0 (#803)

* Update dependencies to v8 internal (#804)

* Delete most demos

* Finish setting up directories

* Basic content swap

* Formatting

* Content swap, start prettier change

* Format

* Copyright headers

* Get basic tests working

* More permissive test timeouts

* Few more updates

* Update to rc

---------

Co-authored-by: Michael Zhen <112977307+zhenmichael@users.noreply.github.com>
Co-authored-by: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com>
Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>
Co-authored-by: Scott Norton <scottnorton@microsoft.com>
Co-authored-by: Sonali Deshpande <48232592+sonalideshpandemsft@users.noreply.github.com>
Co-authored-by: Craig Macomber (Microsoft) <42876482+CraigMacomber@users.noreply.github.com>
Co-authored-by: Michael Zhen <michaelzhen@WIN-1V7LUTHS292.redmond.corp.microsoft.com>
Co-authored-by: nmsimons <nick@dreamlarge.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: runtime Runtime related issues perf
Projects
None yet
Development

No branches or pull requests

6 participants