beyond-all-reason / spring

A powerful free cross-platform RTS game engine
https://beyond-all-reason.github.io/spring/
Other
223 stars 102 forks source link

Fix leaks inside ProfileDrawer and TimeProfiler. #1742

Open saurtron opened 4 weeks ago

saurtron commented 4 weeks ago

Work done

Considerations

See https://github.com/beyond-all-reason/spring/issues/1661 for a discussion of the underlying issue and investigation.

Feel free to discard this solution and do something different. Note even if ProfileDrawer can depend on two frames per second even when minimized, TimeProfiler can't.

Behaviour will be a bit different than currently but I think it should be ok (some records could be old by a factor of DrawTime-UpdateTime, but the difference should be negligible).

Another solution can be simply denying all incoming data when minimized, although this might fail if drawing is not called because of some other condition, just check for globalRendering->active at the top of DbgTimingInfo and also at TimeProfiler, but I think TimeProfiler is supposed to accept records even if ProfileDrawer is disabled or minimized.

sprunk commented 4 weeks ago

Looks nice

saurtron commented 4 weeks ago

Noticed I wasn't doing the cleaning on the thread profiles, so had to add another commit.

I'm removing the lock on draw since doesn't look like it's needed now that the ThreadProfiles are not modified there, please correct me if I'm wrong (other users just push into the structures, unless doing a reset, but that happens only when the Profiler is disabled on debug reset here, or on SetMaximumThreadCount used at SpringApp::InitFileSystem()).

Also, I noticed atm the code is doing:

numThreads = min(profiler.GetNumThreadProfiles(), (size_t)ThreadPool::GetNumThreads()),

but afaics profiler.GetNumThreadProfiles() is always going to be ThreadPool:MaxThreads according to how it's initialized.

Didn't want to change this for consistency with code above in the file, but I believe could do numThreads = ThreadPool::GetNumThreads() directly.

note: Thinking about it, probably not the best since looks like TimeProfiler still accepts data when minimized, and also can be enabled while ProfileDrawer disabled (when run with /debug true false). Will have to rethink this a bit... might be best to clean up the threadProfiles inside TimeProfiler.

saurtron commented 3 weeks ago

I reviewed the threadProfile records cleaning, and turns out the situation is a bit different than with frame data.

Updated the PR and description so it should now properly cleanup both frame and thread records. Also changed title to better reflect the situation.

There is one thing we might want to do different: Currently i'm cleaning both in TimeProfiler:Update() -called every second- and ProfileDrawer:DrawThreadBarCode() -called every frame-. This is to ensure both not too many records accumulate when drawer is disabled and drawer has exactly 0.5 seconds data when drawing them since it really needs them.

Other way to do this to avoid cleaning here would be to check and avoid old records at DrawTimeSlices so we wouldn't need to lock and clean there, downside is DrawTimeSlices also gets applied for frame records but in that case it would also better ensure it's calculating on exactly 0.5 sec data.

note: also noticed TimeProfiler:UpdateRaw (calculates percentages and peaks) is trying to run twice per second, but it's called just once per second because of how Update is scheduled inside Game.cpp. The check should be removed or it's scheduling should be changed, but that's beyond this PR scope so just for the record.