-
Notifications
You must be signed in to change notification settings - Fork 873
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 an interface to ServerValues that lets us read synctrees just in time #2499
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach may be fine, but I need to convince myself that the mutability of SyncTree doesn't create some weird race conditions if multiple increments are stacked on top of each other. Maybe you or @mikelehen can help me convince that this is a non-issue since we only store the latest mutated state in the pendingWriteTree.
@@ -26,6 +26,53 @@ import { ChildrenNode } from '../snap/ChildrenNode'; | |||
import { SyncTree } from '../SyncTree'; | |||
import { Indexable } from './misc'; | |||
|
|||
/* It's critical for performance that we not calculate actual values from a SyncTree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/not/do not/
@mikelehen won't be able to take a look until later today. Unfortunately, that means that we have to revert #2348. We can likely put it back into the the next release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I don't think this just-in-time reading of SyncTree is necessary, and it won't help #2487 since that involves update()
which doesn't hit the SyncTree code path.
I think the root cause is here:
this.serverSyncTree_.calcCompleteEventCache(path), |
It should be:
this.serverSyncTree_.calcCompleteEventCache(path.child(changedKey)),
As-is, we end up calling calcCompleteEventCache() at the root location update() was called on over and over, which in the user's example is /
, so it's going to be expensive (and get more expensive the more writes you have pending).
And since we're applying update()
increments to the wrong existing value, it presumably won't behave properly. So we'll want to add a regression test for that as well.
I'm a bit confused; are you saying that the current proposal is wrong or an over-optimization? In the case that there's a
What do you mean we're applying update() to the wrong existing value? The previous and current code both traverse down the tree; it's just that the last iteration calculated the cache too soon because I needed a full snapshot to be pulled to reuse the snapshot helper function. |
I'm saying:
|
7933b20
to
4a3b18b
Compare
Modified based on @mikelehen's observation that Using the mvp regression test provided by @jmschae in #2487 (thanks!) I was able to confirm that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much cleaner than the previous iteration.
I am not a huge fan of the names, though. I cannot come up with something better, but I am hoping that maybe you can:
class ExistingSnapshotValue implements DeferredExistingValue
class DeferredSyncTreeValue implements DeferredExistingValue
From the names alone, it seems like the implementations do not provide specialized interface implementations, but instead take something away. ExistingSnapshotValue
is no longer deferred, and DeferredSyncTreeValue
is no longer existing. We might need a different name for the interface.
|
||
class ExistingSnapshotValue implements DeferredExistingValue { | ||
private node_: Node; | ||
constructor(node: Node) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: You can drop the suffix for members of classes that are not part of the public API, which will allow you to use constructor property assignments (constructor(readaonly node:Node)
).
Renaming to node
also allows you to get rid of the getter here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I do that I fail to implement the protocol because node
is a value instead of a method.
Based on fix firebase/firebase-js-sdk#2499 to bug firebase/firebase-js-sdk#2487 Micro-benchmarking showed that N writes in succession led to N^2 performance when calcuating the resolved write tree for those writes (thankfully only at the subpath in this SDK). This change matches the optimization in JS that mitigated a 20% performance regression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per offline commends, also renamed the classes ValueProvider, DeferredValueProvider and ExistingValueProvider.
|
||
class ExistingSnapshotValue implements DeferredExistingValue { | ||
private node_: Node; | ||
constructor(node: Node) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I do that I fail to implement the protocol because node
is a value instead of a method.
Based on fix firebase/firebase-js-sdk#2499 to bug firebase/firebase-js-sdk#2487 Micro-benchmarking showed that N writes in succession led to N^2 performance when calcuating the resolved write tree for those writes (thankfully only at the subpath in this SDK). This change matches the optimization in JS that mitigated a 20% performance regression.
* Change FServerValues to just-in-time read from SyncTrees. Based on fix firebase/firebase-js-sdk#2499 to bug firebase/firebase-js-sdk#2487 Micro-benchmarking showed that N writes in succession led to N^2 performance when calcuating the resolved write tree for those writes (thankfully only at the subpath in this SDK). This change matches the optimization in JS that mitigated a 20% performance regression. * Rename types to match JS sdk
* Change FServerValues to just-in-time read from SyncTrees. Based on fix firebase/firebase-js-sdk#2499 to bug firebase/firebase-js-sdk#2487 Micro-benchmarking showed that N writes in succession led to N^2 performance when calcuating the resolved write tree for those writes (thankfully only at the subpath in this SDK). This change matches the optimization in JS that mitigated a 20% performance regression. * Rename types to match JS sdk
* Definition is currently in an "unreleased" extension to FIRServerValues * Tests are added to FData.m (integration tests) * Some tests are renamed to make it clear that ServerValues != timestamps * Functions that resolve server values now need access to any existing snapshot. This is done with lazy evaluation as described below: Based on fix firebase/firebase-js-sdk#2499 to bug firebase/firebase-js-sdk#2487 Micro-benchmarking showed that N writes in succession led to N^2 performance when calculating the resolved write tree for those writes (thankfully only at the subpath in this SDK). Lazy evaluation of current data matches the optimization in JS that mitigated a 20% performance regression.
* Definition is currently in an "unreleased" extension to FIRServerValues * Tests are added to FData.m (integration tests) * Some tests are renamed to make it clear that ServerValues != timestamps * Functions that resolve server values now need access to any existing snapshot. This is done with lazy evaluation as described below: Based on fix firebase/firebase-js-sdk#2499 to bug firebase/firebase-js-sdk#2487 Micro-benchmarking showed that N writes in succession led to N^2 performance when calculating the resolved write tree for those writes (thankfully only at the subpath in this SDK). Lazy evaluation of current data matches the optimization in JS that mitigated a 20% performance regression.
* Definition is currently in an "unreleased" extension to FIRServerValues * Tests are added to FData.m (integration tests) * Some tests are renamed to make it clear that ServerValues != timestamps * Functions that resolve server values now need access to any existing snapshot. This is done with lazy evaluation as described below: Based on fix firebase/firebase-js-sdk#2499 to bug firebase/firebase-js-sdk#2487 Micro-benchmarking showed that N writes in succession led to N^2 performance when calculating the resolved write tree for those writes (thankfully only at the subpath in this SDK). Lazy evaluation of current data matches the optimization in JS that mitigated a 20% performance regression.
Addresses #2487 by creating a new interface that lets us defer reading existing values from a SyncTree until it's actually needed and restricted to the smallest possible scope.