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

Minor optimizations for saving user personal information #30863

Merged
merged 2 commits into from
May 13, 2022

Conversation

CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Jan 26, 2022

  • Remove double hook: the OC_User::changeUser triggers an
    OC\AccountManager::userUpdated and the app is already listening to this
    signal in its Application definition

  • Make createCard not check if a card exists if we already checked
    previously. We also don't try to get the card if the user is disabled
    as we don't use the card in this case

  • Batch insert card properties: instead of doing 6-8 inserts at a time, do all of
    them at the same time (using a transaction)

We this change we go from 102 DB requests to ~81 DB requests when saving
a user email address. Still way too much in my opinion but it's a start :)

Future improvements:

  • Batch modify user properties, currently if you modify 4 properties, each time
    you trigger various hooks to regenerate the addressbook card and the birthday
    calendar, and create an audit log entry, activity entry, ...

@CarlSchwan CarlSchwan added this to the Nextcloud 24 milestone Jan 26, 2022
@CarlSchwan CarlSchwan requested a review from a team January 26, 2022 20:14
@CarlSchwan CarlSchwan self-assigned this Jan 26, 2022
@nickvergessen nickvergessen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jan 26, 2022
@CarlSchwan
Copy link
Member Author

CarlSchwan commented Jan 26, 2022

Todo: fix tests

@CarlSchwan CarlSchwan force-pushed the performance/saving-user-profile-info branch 2 times, most recently from acc00f6 to f01b033 Compare January 27, 2022 10:36
@CarlSchwan
Copy link
Member Author

CarlSchwan commented Jan 27, 2022

TODO check if event is correctly fired in the same situations

Found some cases there the event wasn't fired twice, so I added some more handling

@CarlSchwan CarlSchwan force-pushed the performance/saving-user-profile-info branch 2 times, most recently from da5d57e to 810efb3 Compare February 3, 2022 20:47
@CarlSchwan CarlSchwan force-pushed the performance/saving-user-profile-info branch from 810efb3 to 869a5ba Compare March 2, 2022 20:11
@CarlSchwan CarlSchwan force-pushed the performance/saving-user-profile-info branch from 869a5ba to 830122d Compare March 10, 2022 14:03
@CarlSchwan
Copy link
Member Author

Drone tests are all broken => retriggering them just in case

@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
@blizzz blizzz mentioned this pull request Mar 31, 2022
This was referenced Apr 7, 2022
@blizzz blizzz removed this from the Nextcloud 24 milestone Apr 21, 2022
@blizzz blizzz added this to the Nextcloud 25 milestone Apr 21, 2022
* Remove double hook: the OC_User::changeUser triggers an
OC\AccountManager::userUpdated and the app is already listening to this
signal in its Application definition

* Make createCard not check if an card exists if we already checked
  previously. We also don't try to get the card if the user is disabled
  as we don't use the card in this case

We this change we go from 100 DB requests to 80 DB requests when saving
an user email address.

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
(cherry picked from commit c6fd482)
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan CarlSchwan force-pushed the performance/saving-user-profile-info branch from 830122d to e71db40 Compare May 12, 2022 19:02
@skjnldsv
Copy link
Member

Seems to be still broken

@CarlSchwan
Copy link
Member Author

ci failures unrelared (due to the toast.scss change)

@CarlSchwan CarlSchwan merged commit 9fcf531 into master May 13, 2022
@CarlSchwan CarlSchwan deleted the performance/saving-user-profile-info branch May 13, 2022 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants