HomeWildfire Games

Don't crash when calling the profiler from a 'non-main' thread

Description

Don't crash when calling the profiler from a 'non-main' thread

Motive behind the change: calls to Profile() currently crash unless they are triggered from the main thread, but it's somewhat difficult to know that from the code. It makes more sense to silently ignore those particularly so we can easily have the same code be threaded or not.

This also removes a few profiling calls that don't make much sense.

Differential Revision: https://code.wildfiregames.com/D1853

Event Timeline

elexis added a subscriber: elexis.Jun 14 2019, 9:18 PM

The caller ordered the time spent in that scope to be measured, but he will only get the time spent in the main thread, not the time in the other threads.

If this is a preparation of the Pathfinder threading, then it sounds like measuring the pathfinder is no longer supported, except for things happening in the mainthread.

Figuring out in which code the current code runs, or whether it runs in the MainThread shouldn't be hard to determine, and it would be a better solution to avoid calls that make no sense rather than to ignore them, or to keep the calls and have the calls accounted for.

Figuring out in which code the current code runs, or whether it runs in the MainThread shouldn't be hard to determine, and it would be a better solution to avoid calls that make no sense rather than to ignore them, or to keep the calls and have the calls accounted for.

Doing that just moves the checks to the caller, and I don't believe it to be very useful. Once you've gotten to actually profiling code, you should now what you're doing.
Profiler2 supports multi-threading and ca be used to profile the pathfinder. Profiler1 (aka Profile.h) doesn't. See https://github.com/wraitii/0ad/blame/master/source/ps/Profile.h#L181
TBH

Doing that just moves the checks to the caller, and I don't believe it to be very useful.

With accounting for the calls I actually meant measuring the time spent by that thread (main or not).

If the objective of the profiler is to measure the time of the caller scope, then it should do that for any thread, not only for the main thread.

It might be more work to integrate threading with the Profiler well, so the quick alternative is to not measure the time in threaded parts at all.

That would also be a way to move on without implementing things in full feature scope, but it seems less broken than to measure only the time spent in one thread,
if that part runs in the MainThread,
and if that part does never run in the MainThread, then the call is always ignored, thus the call shouldn't exist to begin with.

Profiler2 supports multi-threading

So use Profiler2 in the Pathfinder - you did that - check, but the silent-ignorance of Profiler1 seems objectionable, it's broken code, and the program should inform the author that their code is broken, rather than not encouraging to consider the use of the calls and to revise them.

This also removes a few profiling calls that don't make much sense.

How does the reader know that they don't make sense? It would depend on the time spent there, if that time is significant, they would match the purpose of the profiler, nay?

It might be more work to integrate threading with the Profiler well, so the quick alternative is to not measure the time in threaded parts at all.

I honestly haven't looked at this much, but I'm not optimistic - Profile.cpp has a lot of C-like code, ugly pointers, things like that.

So use Profiler2 in the Pathfinder - you did that - check, but the silent-ignorance of Profiler1 seems objectionable, it's broken code, and the program should inform the author that their code is broken, rather than not encouraging to consider the use of the calls and to revise them.

As said in the diff summary - my reasoning is that it makes it less tricky to write code that can be threaded or not. That can be objected - perhaps it would be better to crash and let the user handle the threaded-ness. I thought not.

How does the reader know that they don't make sense? It would depend on the time spent there, if that time is significant, they would match the purpose of the profiler, nay?

Indeed, but Profile.h is designed to see average runtime over many frames (that's clear-ish from the implementation, it's also said here).
The calls I removed were either:

  • Profiling stuff that only happens in debug code paths (ConstructLineOnGround, debug overlay) - I don't think that really has a point, and removing profiler calls means that we might also some day remove the profiling headers, which is nice for compilation times.
  • Profiling stuff that happens rarely, thus average time isn't adapted and instead one wants to look at the time taken by individual calls, which Profiler2 is much better at (JumpPointCache reset, Hierarchical Recompute)

So it makes sense to remove them - I could have made that more explicit in the commit, I'll admit.


Bit of a separate topic, but I don't think our in-game Profiler (aka Profile.h) is really that useful. Profiler2 seems objectively superior to it in most respects, to me. Never cared enough to remove it, though.

It might be more work to integrate threading with the Profiler well, so the quick alternative is to not measure the time in threaded parts at all.

I honestly haven't looked at this much, but I'm not optimistic - Profile.cpp has a lot of C-like code, ugly pointers, things like that.

Sure, that might be some amount of work to implement it in a thread-safe way.

As said in the diff summary - my reasoning is that it makes it less tricky to write code that can be threaded or not. That can be objected - perhaps it would be better to crash and let the user handle the threaded-ness. I thought not.

That past tense has a meaning, otherwise you had used present tense?
I agree that it grants the author to ignore what code he writes, but that's not the reasonable way to go, as the developer who knows what his code does is the one creating a better product.

Actually the same argument may be applied to CProfileManager::IsInitialised(), but I don't know all the conditions when it is true.

Profiler2 seems objectively superior to it in most respects, to me. Never cared enough to remove it, though.

I mostly used Profiler1 to measure simulation time from non-visual replay, that's nothing that Profiler2 can do afaik?

/ps/trunk/source/ps/Profile.h
183

@vladislavbelov do you agree that if the author states that a call should be measured, that the call should actually be measured? If one has a call but it's not measured, then it's misleading to have the profiler call there, and the ENSURE notified the developer about that, whereas this condition does not.

That past tense has a meaning, otherwise you had used present tense?
I agree that it grants the author to ignore what code he writes, but that's not the reasonable way to go, as the developer who knows what his code does is the one creating a better product.

Only in that I could change my mind - I don't think it's such a big issue. As said above, I think profiling should only be done if you know what you are doing™, in which case this shouldn't be a problem.
I believe it might be an anti-pattern to have to force the writer of profiling code to think whether he is in a main thread or not - that could be tricky to know, and it seems kind of an expert's debate.
That being said, I'm OK with reverting the "ENSURE" part of this patch if you think it's worth it. Given the profiling call changes, I think it still works.

I mostly used Profiler1 to measure simulation time from non-visual replay, that's nothing that Profiler2 can do afaik?

It can - you just have to auto-start it in the config, iirc. I believe it writes the data automatically when running non-visual replays at the end of execution (assuming things go right, though).