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

Performance Problem with random #612

Open
amyAMeQ opened this issue Jun 6, 2024 · 5 comments
Open

Performance Problem with random #612

amyAMeQ opened this issue Jun 6, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@amyAMeQ
Copy link

amyAMeQ commented Jun 6, 2024

While diagnosing why some of our tests are so slow, I discovered the culprit for us is with random().
We have a function:

public static function randomApiUser(): ApiUser
    {
        $user = self::random()->object();
        return new ApiUser($user);
    }

And it was making tests take five times as long as when we didn't use it.

After poking around, I noticed that rather than getting X random records, it's getting ALL of them and then getting random ones from that.

public function randomRange(int $min, int $max, array $attributes = []): array
    {
        if ($min < 0) {
            throw new \InvalidArgumentException(\sprintf('$min must be positive (%d given).', $min));
        }

        if ($max < $min) {
            throw new \InvalidArgumentException(\sprintf('$max (%d) cannot be less than $min (%d).', $max, $min));
        }

        $all = \array_values($this->findBy($attributes));

        \shuffle($all);

        if (\count($all) < $max) {
            throw new \RuntimeException(\sprintf('At least %d "%s" object(s) must have been persisted (%d persisted).', $max, $this->getClassName(), \count($all)));
        }

        return \array_slice($all, 0, \random_int($min, $max)); // @phpstan-ignore-line
    }

I think it would be more performant if the code used something like this to just get random records from the database (with the appropriate attribute filters):

SELECT * 
FROM table_name
ORDER BY RAND()
LIMIT 1;

I am willing to do a PR for this if you would like.

@nikophil nikophil added the bug Something isn't working label Jun 7, 2024
@nikophil
Copy link
Member

nikophil commented Jun 7, 2024

Hi @amyAMeQ

thanks for the report, you're totally right about this, it could be improved.

Problem is: I think it is not the same syntax between mysql (ORDER BY RAND()) and postgresql (ORDER BY RANDOM())

We're about to release Foundry 2.0, certainly the next week. I'd rather this to be fixed in 2.0 than in 1.x

@nikophil nikophil added enhancement New feature or request and removed bug Something isn't working labels Jun 7, 2024
@amyAMeQ
Copy link
Author

amyAMeQ commented Jun 7, 2024

@nikophil - OK. I will pull the 2.0 branch and play locally. Hoping I can find a way to do it in a database agnostic way.

@nikophil
Copy link
Member

nikophil commented Jun 7, 2024

you'll find an upgrade guide here: https://github.com/zenstruck/foundry/blob/1.x/UPGRADE-2.0.md (not 100% up to date)

BTW don't hesitate to report any problems you'd find in 2.x branch 😅

@kbond
Copy link
Member

kbond commented Jul 24, 2024

Thanks for this report @amyAMeQ, I actually remember when writing that code that this could be a problem but decided to wait until someone reported. Here we are ;)

@nikophil
Copy link
Member

Hoping I can find a way to do it in a database agnostic way

sadly that's not possible. Doctrine does not ship any way to proceed a random. You'll have to add some new DQL function to do this, but we don't want to do this is in Foundry.

then we need to do it with raw SQL, but ordering by rand is slightly different whether if you use MySQL or Postgres.... or Mongo which we actually support! IMO, this would introduce too much complexity....

The first solution that comes in my mind, which is a trade-off, may be to use the query builder (maybe we'll need to split orm and odm behaviors), and select all ids, and then only take few randomly. This is not ideal, but it would prevent some unneeded complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

3 participants