Speed update replies #1046
Speed update replies #1046
Conversation
rmol
requested review from
sssoleileraaa,
kushaldas and
redshiftzero
as code owners
rmol
force-pushed
the
speed-update-replies
branch
5 times, most recently
from
b5c58ee to
debd85a
Compare
| def find_or_create_user(uuid: str, | ||
| username: str, | ||
| session: Session) -> User: | ||
| def find_or_create_user(uuid: str, username: str, session: Session, commit: bool = True) -> User: |
There was a problem hiding this comment.
since this is now only used once where commit=False, should we remove the commit boolean parameter, or do you think we'll need it elsewhere?
There was a problem hiding this comment.
The find_or_create_user function is also called in update_and_get_user, which is called in Controller.on_authenticate_success. If no commit is issued there, the database will be locked and prevent syncing. (The functional tests catch this! 🎉)
We could and probably should merge these two functions, and resolve the commit handling, in a separate PR.
There was a problem hiding this comment.
ah i see, so we could create a new user in the following two cases:
(1) in update_replies if we see that a reply has come in from an admin or journalist who doesn't exist yet in our local db
(2) in on_authenticate_success if the user logging in exists on the server but not yet locally
in case 2 we want to commit since it's a one-off sign in. in case 1, we could be adding many new users for many new replies if it's the first time we are updating our local db against a server that has many users for instance, so we want to delay a commit until the end of a sync in order to improve performance... mmmkay we can refactor later, but this optimization makes sense.
Add boolean "commit" argument to find_or_create_user and update_draft_replies. Also make utils.chronometer log at INFO, so you don't have to run at DEBUG to time things. Debug logging is significantly slower in intensive loops like storage.update_replies, which can take up to a couple of seconds longer with debug enabled, given 1000 sources with two replies each.
rmol
force-pushed
the
speed-update-replies
branch
from
debd85a to
9b63de9
Compare
| def find_or_create_user(uuid: str, | ||
| username: str, | ||
| session: Session) -> User: | ||
| def find_or_create_user(uuid: str, username: str, session: Session, commit: bool = True) -> User: |
There was a problem hiding this comment.
ah i see, so we could create a new user in the following two cases:
(1) in update_replies if we see that a reply has come in from an admin or journalist who doesn't exist yet in our local db
(2) in on_authenticate_success if the user logging in exists on the server but not yet locally
in case 2 we want to commit since it's a one-off sign in. in case 1, we could be adding many new users for many new replies if it's the first time we are updating our local db against a server that has many users for instance, so we want to delay a commit until the end of a sync in order to improve performance... mmmkay we can refactor later, but this optimization makes sense.
Description
A bit more progress toward #1009.
Test Plan
grep durationin the client log,update_repliesshould take around 2.5 seconds on this branch, compared to just over three on theminimize_sync_lookupsbranch upon which this is based.Checklist
If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:
If these changes add or remove files other than client code, the AppArmor profile may need to be updated. Please check as applicable: