dysath / seat-fitting

A Fitting/Doctrine module for SeAT
GNU General Public License v2.0
14 stars 21 forks source link

Optimisation suggestion(s) #93

Open CuboidDroid opened 9 months ago

CuboidDroid commented 9 months ago

Hi

First up, this plugin is awesome. Unfortunately, like many, as our corp has grown in size, we're starting to see a lot more "Last report timed out" errors. I have no idea at his point whether this error is actually a timeout or something else (it happens in under a second, so timeout seems odd as an error?)

I'm still trying to work out the exact cause of that error, however, when I decided to have a quick look at the code - specifically the runReport method in the FittingController - and a couple of optimisations came to mind. They're really simple ones, so hopefully easy enough to add in if they make sense.

First up, if the "fit" should always be "red" if the ship is "red" (I saw ship described as "can fly hull" and "fit" described as "can fly hull with fitting", so this suggests that if ship is red then fit should be red too)... if that's the intent, then you can swap the order of the two inner-most foreach loops so that you check the ship first, and only do the fit loop if canFlyShip is true, otherwise just set canFlyFit to false immediately - and save a whole loop of effort. If you intended to allow for "green" fits on "red" ships, then this first suggestion can be ignored.

Secondly, in both of those inner loops, as soon as you set canflyship or canflyfit to false, you can immediately break out of that loop. The break; command will only break out of the current loop (defaults to one level unless you provide an optional parameter specifying you want to break out of more levels). Basically, there's no point looping over the remaining ships or fits if you've already determined that one of these is false. e.g.

foreach ($fit['skills'] as $skill_id => $level) {
    if (isset($char['skills'][$skill_id])) {
        if ($char['skills'][$skill_id] < $level) {
            $canflyfit = false;
            break;  // add this line to exit this foreach loop immediately and carry on below
        }
    } else {
        $canflyfit = false;
        break;  // add this line to exit this foreach loop immediately and carry on below
    }
}

// execution resumes here after one of the break commands above.

The second suggestion (adding break's to the two innermost foreach loops) should lead to a decent overall speed improvement, but I'm not sure that this is what the original error I mentioned has anything to do with - regardless, it's probably worth adding in.

The first suggestion entirely depends on the intended interpretation of the "Hull" and "Fit" red/green states, but if my interpretation matches your intent, it should also make for a decent speed improvement by completely cutting out the whole fit check.

Hopefully this helps! Thanks again for an awesome plugin - will let you know if I find anything else out. More than happy to hear feedback on what you suspect the actual timeout message refers to too as that might help us figure out possible solutions.

recursivetree commented 9 months ago

Would you mind posting what exact error you get when you see a timeout? I've never seen something like this happening.

CuboidDroid commented 9 months ago

As soon as I can track one down that isn't just the error message in the UI, I'll post details. Unfortunately it is a bit intermittent - fails consistently for a bit, then works for a bit, then fails again, etc. I think the report is 4 fits and about 760 characters (though even with 2 fits and 760 characters seems to have the same issue).

FYI - issue 2 in this post on SeAT Discord mentioned the same issue.

I tried monitoring the logs and looking for errors as suggested by Crypta in that post, but there wasn't anything obvious and nothing new popped up in the log mentioned when the report failed. I'll try again soon and post my findings anyway. Anywhere else I should be looking for errors?

CuboidDroid commented 9 months ago

Ok, so having another look at the code, in /src/resources/views/doctrinereport.blade.php the ajax call to run the report will report the "timeout" error message for any failure result - so 500 errors, for example, will be reported as timeouts, and in my case the server is returning a 500 error when calling the /fitting/runReport/0/{corpId}/{doctrineId} endpoint.

Now I just need to find the right log(s) to figure out if there's anything being logged when this happens.

recursivetree commented 9 months ago

You can use the network section of the browser developer tools to inspect the network request resulting in the 500 error.

CuboidDroid commented 9 months ago

Yeah - unfortunately it does not return any errors - not sure if it's being swallowed by traeffik or something, but it's literally just a 500 error with no detail:

Screenshot 2023-11-20 141013

recursivetree commented 9 months ago

The fact that it says contains an Server header with Apache tells me that it's not traefik, but the seat web container. How long does it take until the error triggers after initiating the request?

CuboidDroid commented 9 months ago

The error triggers almost instantly. Definitely sub 1 second. Errors seem to be getting completely swallowed - can't find any trace of anything on this at all. Kinda at my wits end, unfortunately.