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

Add the ability to process locally available ops prior to returning a container #4793

Closed
christiango opened this issue Jan 12, 2021 · 5 comments
Assignees
Labels
Milestone

Comments

@christiango
Copy link
Member

There arise some scenarios where we are loading a container that puts us in a position that we have a set of ops available locally (i.e we don't need an additional network request to get these ops). Currently, this happens in the ODSP driver, which returns the latest ops on top of the latest snapshot. In the future, we may also cache these in the driver persistent storage (see #4767)

@ghost ghost added the triage label Jan 12, 2021
@vladsud vladsud added the perf label Jan 12, 2021
@vladsud
Copy link
Contributor

vladsud commented Jan 12, 2021

Applying ops from snapshot right away is rather easy. But it will not help much the reason we have this issue - caching of snapshots and client not having data on how far it is behind without making network call.
Given that fetching ops is slower than fetching newer snapshot (in most cases), what's the value we are delivering here (at expense of additional complexity)?

Office_Fluid_FluidRuntime_Performance
| where Event_Time > ago(2d)
| where Data_eventName contains "TreesLatest_end"
| summarize percentile(Data_ops, 50), percentile(Data_ops, 90), percentile(Data_ops, 95)

5.6 16.3 52.5

The way I see it, the only benefit of this work is in conjunction with Issue #4769.
I.e. only when we have ops caching in the pipeline, there is substantial benefit of this work, and it mostly targeted toward scenario of user coming back and not seeing his/her own changes right away, which is confusing and is solved by this proposal.

@vladsud vladsud added this to the March 2021 milestone Jan 12, 2021
@ghost ghost removed the triage label Jan 12, 2021
@christiango
Copy link
Member Author

I certainly think it's most beneficial with #4769.

One case I personally have observed this being beneficial outside of snapshot caching is for create new scenarios. I've often noticed that the first summary is not generated for quite some time, so when doing things like sharing the file or pasting the component URL into chat, you do see the "flash of ops" that is quite jarring. Granted, this could also be solved making sure the number of ops here is minimal by updating snapshotting heuristics.

@vladsud vladsud modified the milestones: March 2021, April 2021 Feb 20, 2021
@vladsud vladsud self-assigned this Feb 20, 2021
@christiango
Copy link
Member Author

@vladsud - I've actually gotten some feedback from folks on our team that the op playback problem is not limited to snapshot caching. In particular, we are getting reports of users seeing the "blank template with placeholders" for the first render and then some op playback to get the latest changes. This is for docs the user has never opened, so snapshot caching is not in the picture.

I'm wondering if in these scenarios the trees/latest call has a good chunk of relevant ops that we should process before handing back the container to the user.

@vladsud
Copy link
Contributor

vladsud commented Mar 30, 2021

Merging into here #4793:

Need to figure out how to apply ops that are coming in snapshot, and do so in generic way not to force other drivers to follow ODSP model.

Some key bugs describing why it's pretty broken for users:
https://office.visualstudio.com/OC/_workitems/edit/4899495
https://office.visualstudio.com/OC/_workitems/edit/4899481

@vladsud
Copy link
Contributor

vladsud commented Apr 22, 2021

This issue is addressed by #5773, as well as 0.38 port - #5850

@vladsud vladsud closed this as completed Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants