Skip to content
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 leaks inside ProfileDrawer and TimeProfiler. #1742

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

saurtron
Copy link
Collaborator

@saurtron saurtron commented Oct 28, 2024

Work done

  • Made ProfileDrawer clean up old frame records on Update() instead of Draw()
  • Made TimeProfiler cleanup its own records on Update (run every second)
    • Also ProfileDrawer will clean them before drawing to make sure it has the right amount of records since it wants 0.5 sec data.
    • Before the fix TimeProfiler can accept threadProfile records even with Drawer disabled when in /debug on off mode. In that case records wouldn't ever be cleaned.

Considerations

See #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
Copy link
Collaborator

sprunk commented Oct 28, 2024

Looks nice

@saurtron
Copy link
Collaborator Author

saurtron commented Oct 29, 2024

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.

This way it can cleanup it's own records when ProfileDrawer is disabled.
Also, call cleaning right before drawing them to make sure to have the right
number of records, since TimeProfiler cleans up just once per second.
@saurtron saurtron changed the title Fix ProfileDrawer accumulating data beyond it's maximum time range. Fix leaks inside ProfileDrawer and TimeProfiler. Oct 29, 2024
@saurtron
Copy link
Collaborator Author

saurtron commented Oct 29, 2024

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.

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

Successfully merging this pull request may close these issues.

2 participants