Announce Refactor

[HDVinnie]

No description provided.

[cbj4074]

@HDVinnie Is the only objective in this particular PR to improve performance?

Or are we interested in slimming-down this controller considerably and delegating most of the functionality to other areas of the application?

If the latter, my preference is that we write tests before moving anything else around.

[HDVinnie]

@HDVinnie Is the only objective in this particular PR to improve performance?

Or are we interested in slimming-down this controller considerably and delegating most of the functionality to other areas of the application?

If the latter, my preference is that we write tests before moving anything else around.

The main purpose at this point is to improve performance. I know some other PHP based trackers out there dont use PHP for announce and instead use https://github.com/Empornium/Radiance or https://github.com/OlafvdSpek/xbt which are written in C++. Now I could be wrong but from what I gather is they use it because it can handle the load better over PHP. But from my tests PHP is not the bottleneck and instead MySQL/MariaDB is and those packages also use MySQL. So idk.

[cbj4074]

Good to know, re: the other options for announcing. A compiled binary will always be fastest, but as you said, the problem doesn't seem to be with PHP, per se.

Furthermore, it looks like there are some custom stats and computations being performed at the announce layer that might be difficult (if not impossible) to replicate in another solution.

What is the EXPLAIN plan for the queries that are so time-consuming? If we can identify specific queries, we can determine why they're so resource-intensive.

[HDVinnie]

Good to know, re: the other options for announcing. A compiled binary will always be fastest, but as you said, the problem doesn't seem to be with PHP, per se.

Furthermore, it looks like there are some custom stats and computations being performed at the announce layer that might be difficult (if not impossible) to replicate in another solution.

What is the EXPLAIN plan for the queries that are so time-consuming? If we can identify specific queries, we can determine why they're so resource-intensive.

There is no longer any slow queries using the default 1 sec.

[cbj4074]

There is no longer any slow queries using the default 1 sec.

Just to clarify, is that true under load?

And were there slow queries at the 1 second setting prior to making the caching changes in this PR?

I suppose I'm asking, "So, we're good?" 😁

[HDVinnie]

There is no longer any slow queries using the default 1 sec.

Just to clarify, is that true under load?

And were there slow queries at the 1 second setting prior to making the caching changes in this PR?

I suppose I'm asking, "So, we're good?" 😁

There was indeed some slow queries prior to the caching. Sometimes the query was slow and sometimes the same query was not. There still is sometimes when mass announcements happen. Like a users client that is seeding 1500 torrents boots or restarts and announces all 1500 at once. I think it just comes down to heavy MySql requests. Jobs/Queues might come in handy here since its one in one out.

[HDVinnie]

I am merging this for now with caching additions. Will revisit after some more testing/logging.

[cbj4074]

What is the rationale behind removing the retrieved() methods from the observers?

I ask because, without those methods, I fail to see how the cache will be populated initially (or after it is cleared).

[HDVinnie]

There is no need to re-issue the cache every time someone retrieves the model in question. Its a problem on high traffic sites and causes load issues. I saw a 3-4 second increase in page load times for users and torrents when there 500+ ppl browsing the site. We already cover created, updated, deleted and restored. retrieved serves no purpose as created and updated cover when a user or torrent is initially created or updated.

However you are right about when someones Redis gets cleared. I think if anything Cache::add like I originally had will be best for retrieved. As then it only puts the cache if doesn't already exist. Using Cache::put on the retrieved event makes no sense as its heavy load and is repeating what created and updated cover .

[cbj4074]

My point is that the model in question should be retrieved only once, by the very first person to request it on a clean cache. After that, the model should be cached and then all subsequent attempts to retrieve it should receive it from the cache.

That said, I think I understand your point and see the problem, which is that the Announce controller is not the only place in which the Torrent model data is requested. If that's the case, then those other places should also be fetching from the cache and not hitting the DB every single time.

That's why all of these models should have repositories that provide a level of abstraction; then, the repository could handle the caching completely transparently, so that the caller doesn't have to know or care how the caching is implemented; it just requests the object from the repository and the repository itself decides whether to return a cached version or a live version, depending on the context.

[cbj4074]

It looks like you restored the retrieved() method with Cache:add(), which, as you suggested, is the best short-term solution.

That said, let's keep the repository pattern implementation on the radar.

[HDVinnie]

Agreed.