Make WordPress Core

Opened 14 years ago

Last modified 41 minutes ago

#18836 assigned enhancement

ORDER BY RAND() is slow

Reported by:scribu's profile scribuOwned by:pbearne's profile pbearne
Milestone:6.9Priority:normal
Severity:minorVersion:
Component:QueryKeywords:early has- has-test-info dev-feedback
Focuses:performanceCc:

Description (last modified by scribu)

WP_Query currently accepts 'orderby' => 'rand' which translates to ORDER BY RAND().

This is very slow when you have many posts, since it effectively calls RAND() for each row.

A faster way would be to call RAND() only once and put it in the LIMIT clause.

The only thing is that we have to make sure that the generated number is smaller than (total number of posts - number of posts to fetch).

So, this would require to do an extra query to calculate the total. It should still be faster than the current method.

If we want to get more than one post, we can get them in any order and then call shuffle() on the resulting array.

Attachments (1)

18836.diffโ€‹ (976 bytes) - added by MisdaX 10 years ago.
first attempt - passes all phpunit standard tests

Download all attachments as: .zip

Change History (35)

#1 @scribu
14 years ago

Here's a method of doing it all in a single query:

โ€‹http://explainextended.com/2009/03/01/selecting-random-rows/

Not sure how feasible it would be for WP.

#2 @sirzooro
14 years ago

  • Cc sirzooro added

#3 @scribu
14 years ago

  • Description modified (diff)

#4 @c3mdigital
12 years ago

  • Keywords needs- added

#6 @nacin
11 years ago

  • Component changed from Performance to Query
  • Focuses performance added

#7 @wonderboymusic
11 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

A for this would be fun.

@MisdaX
10 years ago

first attempt - passes all phpunit standard tests

#8 @MisdaX
10 years ago

Tests with MySQL EXPLAIN shows no creation of temporary tables. The execution time with ~900 posts is also 2-3 times faster.

I ran the standard phpunit tests - no additional error was found.

#9 @MisdaX
10 years ago

  • Keywords has- added; needs- removed

#10 @MisdaX
10 years ago

  • Keywords needs-testing added

#11 @johnbillion
10 years ago

Looks like your got missed, MisdaX. Sorry about that.

Can we confirm which unit tests cover the ORDER BY RAND() functionality please? Not that it's really very testable, but still.

#12 @MisdaX
10 years ago

It is indeed difficult. I tried to reduce the impact of the as best as I can. I tested a vanilla wordpress and a ed wordpress with the whole unit test package to ensure that I broke nothing.

I assumed that there are tests which cover the ordering part of the function. No change in the result of the unit tests was a sign for me that everything should be ok. Unfortunately I have no idea how I can provide a suitable test for a random order functionality :-)

Independently, it would be nice if someone could provide a performane test with a big database.

This ticket was mentioned in โ€‹PR #6241 on โ€‹WordPress/wordpress-develop by โ€‹@pbearne.


14 months ago
#13

  • Keywords has-unit-tests added; needs-unit-tests removed

This update modifies the 'test_orderby' function assertion to accommodate expected 'rand()' value in the SQL query request. An additional unit test 'test_wp_query_orderby_rand' is also introduced to verify that the 'orderby' => 'rand' parameter provides seemingly random results upon multiple requests.

Trac ticket: 18836

#14 @pbearne
14 months ago

  • Keywords has- removed
  • Owner set to pbearne
  • Status changed from new to assigned

Hi

I created a test to check we get a random order.

but the code provided in the did not provide a random order so more work is needed.

Last edited 14 months ago by pbearne (previous) (diff)

#15 @pbearne
14 months ago

may put into another ticket

This ticket was mentioned in โ€‹Slack in #core-performance by pbearne. โ€‹View the logs.


14 months ago

#17 @pbearne
6 months ago

  • Milestone set to Future Release

#18 @pbearne
4 months ago

  • Keywords has- added
  • Milestone changed from Future Release to 6.8

This ticket was mentioned in โ€‹Slack in #core by audrasjb. โ€‹View the logs.


3 months ago

#20 @audrasjb
3 months ago

  • Milestone changed from 6.8 to 6.9

As per today's bug scrub: It's a bit late in the release cycle to introduce Query/DB changes, so I'm moving it to 6.9 with the early keyword.

Please note that there is a WPCS issue in the current PR.

#21 @audrasjb
3 months ago

  • Keywords early added

This ticket was mentioned in โ€‹Slack in #core-performance by adamsilverstein. โ€‹View the logs.


7 weeks ago

#23 @adamsilverstein
7 weeks ago

We discussed this today in the Performance bug scrub and the question came up: is this still true: "ORDER BY RAND() is slow" or has this issue been resolved already?

#24 @pbearne
7 weeks ago

@adamsilverstein I don't know It may have already fixed upstream

#25 @SirLouen
3 weeks ago

I've passed my WP-Query benchmarking template and here are the results:

https://i.imgur.com/0RPY1sg.png

These are the 3 queries benchmarked in a pool of 10K posts, $post_per_page = 1K, 10 iterations each

new WP_Query([
    'post_type' => 'post',
    'posts_per_page' => $posts_per_page,
    'orderby' => 'rand',
    'no_found_rows' => true,
    'cache_results' => false
]);

new WP_Query([
    'post_type' => 'post',
    'posts_per_page' => $posts_per_page,
    'orderby' => 'date',
    'order' => 'DESC',
    'no_found_rows' => true,
    'cache_results' => false
]);

new WP_Query([
    'post_type' => 'post',
    'posts_per_page' => $posts_per_page,
    'orderby' => 'title',
    'order' => 'DESC',
    'no_found_rows' => true,
    'cache_results' => false
]);

Tomorrow I will be checking the new implementation pushed by @pbearne to see if it actually improves the rand implementation and create a proper testing report.

#26 @SirLouen
3 weeks ago

  • Keywords changes-requested added; needs-testing has-unit-tests has- removed

Combined Issue Reproduction and Test Report

Description

โŒ This report can't validate that the indicated works as expected.

tested: โ€‹https://.com/WordPress/wordpress-develop/pull/6241.diff

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.4.6
  • Server: nginx/1.27.5
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.4.6)
  • Browser: Chrome 135.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty-Five 1.2
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0
    • WQROC 1.0.0
    • WP Query Benchmarking 1.0.0

Expected Results with

  1. Random sort works.
  2. Benchmark indicates improvement.

Actual Results

  1. โŒ Random sort fails with the
  2. โœ… There is a slight performance improvement

Additional Notes

There are some things wrong with this :

  • Firstly, as shown here, there is no randomization with this :

Before (without ): โ€‹https://streamable.com/p6mjct
After (with ): โ€‹https://streamable.com/dnf6ob

  • Secondly, as we can see in the screenshots attached, the performance is slightly improved.
  • Third, the unit test is always returning OK because it's technically not checking for randomized queries adequately. Ideally, it should generate a ton more posts because with such little size: 10 interactions for only 3 units. In case that the unit test was right, it could be triggering false asserts once in a while in the automated protocol (i.e. Actions). This could be awful because people, unrelated to this , could go crazy trying to figure out why this test actually failed for them on an unrelated topic, so minimizing or completely ditching this is ideal in this case. Apart to that, as I say, the test itself wrong because it triggers and fully breaks too early.
  • Finally, there are already some unit tests aimed to orderby rand like test_orderby() so this new test could be, moreover, redundant.

Further research on this algorithm is required.

Supplemental Artifacts

Last edited 3 weeks ago by SirLouen (previous) (diff)

#27 @SirLouen
3 weeks ago

  • Keywords close added

Adding more beef to the grill, I completely forgot the most obvious testโ€ฆ number of queries

This is without any modifications, just bare queries like the mentioned here

https://i.imgur.com/CQdFQij.png

After all this testing, I'm starting to think that this ticket is completely obsolete by now.

With all this info provided, I would add this as a candidate for close โ‡’ wont-fix

#28 @SirLouen
3 weeks ago

  • Keywords close removed

After hour and a half trying to understand โ€‹the algorithm, I think I've come with a solution with the idea that @scribu was suggesting one decade ago, and I think I got it.

The performance gains are important:

https://i.imgur.com/49JjRi1.png

I'm still double-checking the code, but I will provide a tomorrow, just wanted to remove the close proposal because maybe there is an opportunity for optimization here.

Test Demo: โ€‹https://streamable.com/m5iklq

Last edited 3 weeks ago by SirLouen (previous) (diff)

This ticket was mentioned in โ€‹PR #8715 on โ€‹WordPress/wordpress-develop by โ€‹@SirLouen.


3 weeks ago
#29

  • Keywords has- added

Brief Testing Info Ideas

  1. First you might need to publish a lot of posts. The more, the better, 10 for example
  2. Use a Query like:
    $query = new WP_Query([
                'posts_per_page' => 10, 
                'fields' => 'ids',
                'post_status' => 'publish',
                'orderby' => 'ID',
                'order' => 'ASC'
    ]);
    
  1. And fix the result (because you need to check if future WP_Queries return the same order or not)
$fixed_ids = $query->posts;
  1. Finally we generate another WP_Query, but this time with the orderby and rand for the exact same posts we got previously. Do this multiple times, check generation order to see if there is some sort of randomness in each generation.
$query = new WP_Query([
        'post__in' => $fixed_ids,
        'posts_per_page' => 10,
        'orderby' => 'rand',
        'post_status' => 'publish',
        'ignore_sticky_posts' => true,
        'fields' => 'ids',
    ]);
  1. For the performance part, use microtime and get_num_queries() accompanied by define( 'SAVEQUERIES', true );

Trac ticket: https://core.trac.wordpress.org/ticket/18836

#30 follow-up: @SirLouen
3 weeks ago

  • Keywords has-testing-info dev-feedback added; changes-requested removed

Note that I have ignored the unit-tests because there are already unit-test for orderby rand in ::test_orderby()

#31 in reply to: โ†‘ 30 @pbearne
3 weeks ago

Replying to SirLouen:

Note that I have ignored the unit-tests because there are already unit-test for orderby rand in ::test_orderby()

Good work @SirLouen

No, let's get this in core

#32 @SirLouen
3 days ago

Testing Instructions Improved

  1. Use this plugin in a new clean environment: โ€‹https://.com/SirLouen/wp-query-benchmarking-v2
  2. First generate 10K posts to have a populated DB
  3. Run the performance tests with and without the .

You are welcome to review and improve the plugin to add more test cases or improve the mock data.

#33 @wordpressdotorg
14 hours ago

  • Keywords has-test-info added; has-testing-info removed

This ticket was mentioned in โ€‹Slack in #core-test by sirlouen. โ€‹View the logs.


41 minutes ago

Note: See TracTickets for help on using tickets.