[ASDataController] Add an experiment that avoids flushing editing queue before starting the data pipeline pipeline #1564
Conversation
…controller pipeline Step 1 of the pipeline is on main thread and needs pendingMap which was updated by the end of step 1 of the run. And step 1 operates on data source index space. So it should be ok to start it ASAP -- without waiting for any running work on the background editing queue. One potential race condition after this change is that it's possible for main thread to query the data source (step 1) while the editing queue consumes the last change set (step 3). However, as long as the client's each nodeBlock and the node's layout code capture/reference an individual model object (as opposed to the whole data set) -- which is what clients are supposed to do anyways, then everything should be fine. The benefit of this diff is that the pipeline will be able to accept many more change sets within a short time window, for example when clients submit a burst of separate small updates. I tested this diff against our test suite several times and smoke tested it in our code base without any issues.
nguyenhuy
changed the title
|
I could be wrong, but I believe this was originally added by @appleguy so I'd like his review on it (pinged) but I agree this wait in principle should not be here. |
|
Yeah, this was added long time ago, before we refactored this class. |
|
Yes, I did add this :). To be honest, I haven't followed the subsequent changes closely enough to say with confidence that it is safe to remove. However, I am absolutely confident that we should gate this with an experimental flag. This would be too risky for our team to merge un-gated, and it would be a bummer to have to add a patch. Additionally a flag would let our teams validate / verify the extent of the win from removing this synchronization point (likely very significant as some app flows happen to do two edits sequentially, like adding content and removing a loading indicator). It would be great to remove this entirely, ASAP. If we can verify it is safe in production, we should definitely remove it from the code & experimental options list. |
|
Agree, we need to wrap this in an experiment. Wanted to make sure I don’t miss anything obvious before doing so. |
nguyenhuy
changed the title
|
@appleguy @Adlai-Holler Wrapped in an experiment and update our tests to test it. Can I have an approval? I'll hook this one up with our experiment framework. Would be great if you can do the same so we'll have better coverage. |
|
Thanks for the reviews everyone! |
Step 1 of the pipeline is on main thread and needs pendingMap which was updated by the end of step 1 of the run. And step 1 operates on data source index space. So it should be ok to start it ASAP -- without waiting for any running work on the background editing queue.
One potential race condition after this change is that it's possible for main thread to query the data source (step 1) while the editing queue consumes the last change set (step 3). However, as long as the client's each nodeBlock and the node's layout code capture/reference an individual model object (as opposed to the whole data set) -- which is what clients are supposed to do anyways, then everything should be fine.
The benefit of this diff is that the pipeline will be able to accept many more change sets within a short time window, for example when clients submit a burst of separate small updates.
I tested this diff against our test suite several times and smoke tested it in our code base without any issues.