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

recount command seems unnecessarily slow #119

Open
vesper8 opened this issue Sep 19, 2019 · 4 comments
Open

recount command seems unnecessarily slow #119

vesper8 opened this issue Sep 19, 2019 · 4 comments

Comments

@vesper8
Copy link

vesper8 commented Sep 19, 2019

In my application I have about 10k reacters and 25k reactants.

If my love_reactions table is completely empty, issuing the recount command still takes an incredibly long time.. almost 1 full minute. Same thing if my love_reactions table contains just a dozen or two reactions

I can only assume that the recount command is for some reason iterating over all reacters or reactants, or both.. but why would it do that? Isn't the source of truth in this case the love_reactions table? If the love reactions table is empty, then the recount should be incredibly fast?

On this note, is there a method for clearing all reactions from a specific reacter? Say I want to delete a reacter (a user) and I wants all of its reactions cleared from the tables. Currently I'm doing it like this:

\DB::connection('data')
    ->table('love_reactions')
    ->where('reacter_id', $user->getLoveReacter()->id)
    ->delete();

And then I'm issuing the recount.. which takes way too long IMO, specially if the above command just basically truncated the love_reactions table as it does in my test case

Lastly.. could you make it possible to call the command from a controller? Right now I'm doing this Artisan::call('love:recount'); But I find that doing this kind of call from a controller is a bit hacky. At least if I could dispatch the job from a controller. But the problem is you've added all the logic in the command class, instead of creating a command that dispatches a job and putting the logic in the job class, so it may be dispatched from other places. That's how I usually do it in my projects.. separating the commands and the jobs.. specially if a lot of processing/logic is involved such as in this case

Food for thought.. thanks for consideration

@antonkomarev
Copy link
Member

antonkomarev commented Sep 20, 2019

Yeah, recounting is slow because in current implementation artisan love:recount command iterating thru the Reactant models and determine if Reactable model type required to be recounted, then collects and count all the reactions of it.

If we will collect all the reactions as you proposed, then to determine if Reactable model should be recounted we can use love_reactants.type column to filter out not needed reactions. Seems good to me.

If you are using foreign keys - then all love_reactions should be deleted automatically on love_reacters record being deleted. But if you delete reacter directly from database or using DB facade - then counters & total wouldn't be affected, because Eloquent Events wouldn't be fired.

It's a good idea to have service class which will be called in the job.

To summarize everything:

  1. we need to refactor artisan love:recount command counting logic
  2. we need to create service class to recount reactions of all the reactants (with filtering possibilities)
  3. we need to think about ability to recount reactions of reactants which were reacted by exact reacter (to not recount all application reactions; maybe it will be included in 2nd point of this list).

@antonkomarev antonkomarev added this to the v8.2.0 milestone Sep 20, 2019
@sburkett
Copy link
Contributor

sburkett commented Nov 10, 2019

Q: Is this count/recount approach only needed if you are trying to use aggregated states, weighted approaches, etc?

If the answer is "Yes", then ignore the following. If the answer is "No", can you comment on the information below?


I am curious as to why we need need this type of approach at all, especially if you are not using "weights". I get that it is much easier to query a static total from a simple table like love_reactant_reaction_counters etc., but this seems like overkill for simple counts of "likes" or "dislikes".

I haven't converted this to Eloquent yet, but why not just query them on-demand and optionally cache them?

SELECT COUNT(love_reactions.id) AS total, love_reaction_types.name
FROM love_reactions
LEFT JOIN love_reaction_types ON love_reaction_types.id = love_reactions.reaction_type_id
WHERE reactant_id = ?
GROUP BY reaction_type_id

e.g.

https://github.com/dwightwatson/rememberable
https://dev.to/kovah/laravel-5-use-query-caching-to-make-your-app-really-fast-gk9

Or, just use the native cache driver in Laravel to store anf manage the totals manually. Either way seems more more real-time and efficient than the current approach.

Another issue if you are using a basic Like/Dislike approach is that the totals are wrong if you allow someone to change their vote. The totals keep climbing as people flip-flop their votes. Ostensibly this recount approach would remedy that, but I haven't tried it. But again, seems like overkill for a basic Like/Dislike approach without weights, etc.

Another approach would just be to have the proper model relationships in place and just use Eloquent:

$likes = $comment->likes->count();
$dislikes: $comment->dislikes->count();

I could be wrong here in the context of Laravel/Love, but if I am missing something, please let me know!

@antonkomarev antonkomarev modified the milestones: v8.2.0, v8.3.0 Jan 29, 2020
@antonkomarev
Copy link
Member

This task is very important but too complicated. I've tried to refactor it for the next minor version, but it wasn't successful. Will try to look on it one more time after v8.2 release.

@antonkomarev
Copy link
Member

antonkomarev commented Feb 8, 2020

I can only assume that the recount command is for some reason iterating over all reacters or reactants, or both.. but why would it do that? Isn't the source of truth in this case the love_reactions table?

There is a reason behind this logic.

If we will collect all the reactions on command run, sum them and add to the reactants - there is a possibility that during the execution of the command, another reaction will be added to some reactant and we will not take it into account when rebuilding counters. Then data will be not relevant.

Right now we are collecting all the reactants, and fetching it's reactions right after the counters reset. Even if new reactants will appear during the execution of the command - we wouldn't care because their reactions wouldn't be affected.

@vesper8 I will be glad to hear another ideas how we could solve it more quick and safe way.

You could always delete reactions using Eloquent model methods and you wouldn't need to rebuild counters then:

$reactions = Reaction::query()
    ->where('reacter_id', $reacter->getId())
    ->get();

foreach ($reactions as $reaction) {
    $reaction->delete();
}

@antonkomarev antonkomarev modified the milestones: v8.3.0, Backlog Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants