Opened 6 years ago
Last modified 4 weeks ago
#44921 accepted defect (bug)
User nicename discovery is slow to return
Reported by: | spacedmonkey | Owned by: | pbearne |
---|---|---|---|
Milestone: | 6.8 | Priority: | normal |
Severity: | normal | Version: | 2.0 |
Component: | Users | Keywords: | has-patch needs-testing has-unit-tests needs-refresh |
Focuses: | performance | Cc: |
Description
In the function wp_insert_user
it checks to is if the user nicename is in use by another user before saving. This is because, a nicename should is required to be unique. Unlike usernames, however, that rejects already in use usernames, if first checks to see if the nicename is in uses, then runs a while loop to try and find the next available nicename with a suffix. So nicename jonny becomes jonny_2 if jonny is already in use.
The behaviour is fine, however it uses a while loop to find the next available nicename, which can result in an extremely high number of queries. Take the following use case.
You have a script that generates 10000 test users for a site. The call to wp_insert_user, passed 'user_nicename' => 'test_user'
. Within the function call, it will run the while loop and find next available nicename. By the 9999th call, it would have to loop around 9999 to find the next slot, making the return of wp_insert_user extremely slow.
For testing scripts and sites with lots of registered users, this could make wp_insert_user
almost unusable.
Attachments (3)
Change History (28)
#1
@
6 years ago
I would recommend, using get_user_by('slug', $usernice);
instead of a raw SQL query, as it would at go through caches.
#2
@
6 years ago
- Keywords has-patch added
I did a first pass at this 44921.diff.
This doesn't add caching, but just replaces the raw calls to database with function calls to get_users
. Now that this is going to core, if give us more options to cache upstream.
#3
@
6 years ago
- Keywords needs-testing added
The patch 44921.2.diff adds a new helper function nicename_exists
which is similar to email_exists
.
This is a much cleaner way of calling this, my only issue with it is the kind of odd, function signature of nicename_exists
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
5 months ago
#6
@
5 months ago
- Keywords needs-refresh added
This ticket was discussed in today's performance bug scrub.
Let's update the patch and move this edge case to the finish line.
Props to @joemcgill and @adamsilverstein.
This ticket was mentioned in PR #6739 on WordPress/wordpress-develop by @pbearne.
5 months ago
#7
- Keywords has-unit-tests added; needs-refresh removed
The nicename_exists
function is implemented to determine if a given user_nicename exists. The function is also integrated into the user nickname checking mechanism within the user class. Additionally, various test cases for the new function have been added, which cover scenarios with existing, non-existing, and nicknames associated with different user logins.
#8
@
4 months ago
- Milestone changed from Awaiting Review to 6.7
- Owner set to pbearne
- Status changed from new to accepted
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
3 months ago
This ticket was mentioned in Slack in #core-test by mukeshpanchal27. View the logs.
2 months ago
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
2 months ago
@mukesh27 commented on PR #6739:
8 weeks ago
#15
@peterwilsoncc If you get chance please review it as it's so close.
#16
@
8 weeks ago
As the purpose here is to ensure the user nicename is unique, I think introducing wp_unique_user_nicename()
would be a better option to match this similarly named functions for post and term slugs.
I'm also curious to know the real world improvements available here. While inserting 1000 users with the test_user
as the slug will result in some performance impact, I don't think it reflect reality.
In the case of WordPress.org if someone enters the nicename peterwilsoncc
then the most likely situation is one or two queries before a unique suffix is found. peter
would result in a few more queries but I don't see how querying the full user object rather than only the nicename is much of a n improvement.
@mukesh27 commented on PR #6739:
7 weeks ago
#18
I did some benchmarking for current trunk and the PR changes with 100 and 1000 user inserts and the PR didn't show any performance regression instead it improve the performance if we use same nicename.
Result:
Trunk: Total time to insert 100 users: 2.0517628192902 seconds. PR: Total time to insert 100 users: 1.4436409473419 seconds. Trunk: Total time to insert 1000 users: 80.944605112076 seconds. PR: Total time to insert 1000 users: 16.288486003876 seconds.
Could anyone of you please benchmarking your end?
@peterwilsoncc commented on PR #6739:
7 weeks ago
#19
I did some benchmarking for current trunk and the PR changes with 100 and 1000 user inserts and the PR didn't show any performance regression instead it improve the performance if we use same nicename.
This isn't the question I am asking on the trac ticket. I am asking what the real world improvements are.
I accept that this change will improve performance of inserting 1000 users with the identical nice name test_user
but that doesn't reflect the real world situation in which people are likely to use their actual name for the nice name.
Even common names such as John or Peter aren't likely to result in the need to iterate thought 100 different user accounts to find a suitable suffix.
@mukesh27 commented on PR #6739:
7 weeks ago
#20
Got it but for bigger user base site have more then 100 same nicename so i would be useful there.
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
6 weeks ago
This ticket was mentioned in Slack in #core by stoyangeorgiev. View the logs.
5 weeks ago
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
4 weeks ago
This ticket was mentioned in Slack in #core by chaion07. View the logs.
4 weeks ago
#25
@
4 weeks ago
- Milestone changed from 6.7 to 6.8
Thanks @spacedmonkey for reporting this. We reviewed the Ticket during a recent bug-scrub session and based on the feedback we are updating the milestone to the next release (6.8). Any contributor can still work on this and merge in time by reverting back if they manage to do so.
Props to @khoipro
Cheers!
An example of where this issue is happening in the wild.