Skip to content
This repository has been archived by the owner on Sep 13, 2022. It is now read-only.

Fix Monocle/Alternative out of memory with KEEP_GYM_HISTORY #78

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

j16sdiz
Copy link

@j16sdiz j16sdiz commented Jan 30, 2018

When KEEP_GYM_HISTORY is on in Monocle/Alternative config, the fort_sighting and raid may contain multiple records of the same gym. Only the latest gym / fort status should be retrieved to avoid out of memory problem.

LEFT JOIN fort_sightings fs ON fs.fort_id = f.id
LEFT JOIN raids r ON r.fort_id = f.id
LEFT JOIN fort_sightings fs ON (fs.fort_id = f.id AND fs.last_modified = (SELECT MAX(last_modified) FROM fort_sightings fs2 WHERE fs2.fort_id=f.id))
LEFT JOIN raids r ON (r.fort_id = f.id AND r.time_end >= extract(epoch from now()))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend using a PHP function to get time() and pass that through as a parameter as extract(epoch from now()) is not supported on MySQL, and conversely UNIX_TIMESTAMP() is not supported in Postgres

When `KEEP_GYM_HISTORY` is on in Monocle/Alternative config, the `fort_sighting` and `raid` may contain multiple records of the same gym. Only the latest gym / fort status should be retrieved to avoid out of memory problem.
@j16sdiz
Copy link
Author

j16sdiz commented Jan 30, 2018

@CalamityJames fixed

Copy link
Contributor

@123FLO321 123FLO321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working fine for me with KEEP_GYM_HISTORY true

Copy link
Owner

@Glennmen Glennmen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR should get merged asap, more people are running Monocle Alternative with KEEP_GYM_HISTORY.
The current queries are optimised for Monocle Alternative with KEEP_GYM_HISTORY = false, so we would want to keep those queries for performance improvement.

I was thinking about a config option to switch between KEEP_GYM_HISTORY true/false queries, unless you have a better suggestion.

@j16sdiz
Copy link
Author

j16sdiz commented Feb 26, 2018

Actually, the r.time_end >= :time condition save time on both with history and no history case.
It's the condition on fs.last_modified that may harm no history performance.

@Glennmen
Copy link
Owner

@j16sdiz I think you are right about r.time_end >= :time
Do you think you can add the config option for the other part? (Default for KEEP_GYM_HISTORY = false)

@123FLO321
Copy link
Contributor

last_modified and fort_id are indexed.
I don't think this would impact the ones who don't use KEEP_GYMHISTORY

Can someone with KEEP_GYMHISTORY = False run a performance test on:

SELECT *	
  FROM forts f
  LEFT JOIN fort_sightings fs ON (fs.fort_id = f.id AND fs.last_modified = (SELECT MAX(last_modified) FROM fort_sightings fs2 WHERE fs2.fort_id=f.id))
  WHERE f.id = 1;

vs

SELECT *	
  FROM forts f
  LEFT JOIN fort_sightings fs ON fs.fort_id = f.id
  WHERE f.id = 1;

I did a test with f.id = 1...20 (uncached) on this and got the following result
new avg: 0.071s
old avg: 0.066s

@j16sdiz
Copy link
Author

j16sdiz commented Feb 27, 2018

@Glennmen fixed

@j16sdiz
Copy link
Author

j16sdiz commented Feb 27, 2018

@123FLO321 There is a 6% to 8% perfomance degrade.
Personally I don't think this worth the extra code for a few milliseconds gain.
But I respect the project owner.

if ($alternateKeepGymHistory) {
$query = str_replace(":fort_condition", "fs.last_modified = (SELECT MAX(last_modified) FROM fort_sightings fs2 WHERE fs2.fort_id=f.id)", $query);
} else {
$query = str_replace(":fort_condition", "1=1", $query);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the 1=1 check.
Maybe include the AND inside :fort_condition and replace it with an empty string if $alternateKeepGymHistory is false?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants