-
-
Notifications
You must be signed in to change notification settings - Fork 87
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
fix(game): show correct softcore users count in Achievement Distribution graph #2466
fix(game): show correct softcore users count in Achievement Distribution graph #2466
Conversation
There are a lot of changes in this branch that seem unrelated to the achievement distribution issue. I'm not sure what happened - perhaps We should thin out this branch so only your specific changes are included, though. |
Yep, composer fix definitely seemed to have went above and beyond. I can see if I can pull out those changes. |
Initial commit and composer fix Update achievement distribution selections and migration to Eloquent Update calculateBuckets and handleAllAchievementsCase to accomodate selection update Update V1Test to accomodate selection update Composer fix
af5d59d
to
69d5e9c
Compare
This reverts commit 69d5e9c.
@wescopeland additional changes added by |
Doing some local testing, the new implementation appears to be an order of magnitude slower than what's currently in These queries are taken from a game that has 175,000 |
Got it. I can investigate the SQL as well to see if there's any way to optimize it. What's curious is that this query pulls from player_games, which seems to have 1 row per player per game, instead of player_achievements which is 1 row per achievement per player. There wouldn't be a SQL statement I could run locally that would insert 200k rows into player_achievements would there? :) |
Ah, truthfully I didn't even notice that these queries aren't reading from |
No worries! I'm going to break this off into another branch to play around with. Hopefully I can figure something out that's more performant. 😁 |
Hey @wescopeland ! I did some extensive testing/tinkering with the Achievement Distribution query and as the data is currently structured, I'm not sure we'd be able to get the selection much faster than it currently is. The fastest I could get it was 50ms for 25k rows on the That said, there is a different approach we could take to speed this up, but it would require altering the I'm not sure if there is a configuration difference with my instance of MySQL but the Alter + Index seems to have equivalent performance compared to the legacy query. For your reference, here is the SQL I used for the alter and the index build:
Again, this can have a big impact since it alters a table with potentially a million+ rows in production, and it would calculate the softcore unlocks for each one of those (one time). Once altered, the column would be calculated for a row upon insert or recalculated upon update. With that in mind, it's completely understandable if this is something RA would not want to pursue at the moment. |
I had a feeling needing a new denormalized column would be how things might shake out 😆 That said, I would be fine with pursuing this. Before moving forward, I think another maintainer should also sign off on the idea though (cc @RetroAchievements/web-maintainers). I can think back to other times where I've needed to perform a calculation to peek into a user's mixed progress. Just having the raw value would've been more helpful and performant. Going down this road is a more significant lift and is much more fraught with peril. It would need to occur in two phases (releases): a write phase and a read phase. For the write phase, we would need a PR/changeset which includes:
I personally haven't triggered a full resync of player game metrics on the server, but I know we've done it once or twice in the past when we first began experimenting with the performance benefits of denormalized data. Once the code is deployed, we'd trigger a full resync, which would queue up probably about 10,000,000 jobs. This would take quite some time to finish. For the read phase, it'd essentially be a PR containing what you have now, but instead reading from these new denormalized values. |
Great! I can start researching the write phase steps this week. I may reach out for guidance on a few things if I hit a wall, but I'll be sure to let you know. |
@wescopeland a PR for the Write Phase (#2477) has been submitted. If/when the new PR is approved and merged, I can update this PR to accommodate the changes. Just let me know if there's anything I may have missed, or if you have any questions/concerns. Thanks! |
@wescopeland circling back to this and after some testing, it seems like the automatic "Soft Delete" check when selecting from the Eloquent with Soft Delete check:
Eloquent without Soft Delete check (code same as above):
DB::table() select without Soft Delete check:
|
@TwosomesUP Sorry for my late reply here. I'm in the process of setting up some production data to test with, which should be done later today. As soon as that is set up, getting you a review will be my highest priority 👍 |
I appreciate it Wes, but no rush! I still need to update this pull request with the new selects that account for the new I've been on vacation, so I haven't had a chance to get to it yet, but I'll try to get that pushed either today or tomorrow. |
Hi @TwosomesUP! Looks like we may have a small merge conflict. Then, assuming @Jamiras approves, we can merge to get this in the next release. |
Initial commit and composer fix
Update achievement distribution selections and migration to Eloquent
Update calculateBuckets and handleAllAchievementsCase to accommodate selection update
Update V1Test to accommodate selection update
Resolves: #2399