search

FEniCS / dolfinx

Next generation FEniCS problem solving environment
https://fenicsproject.org
GNU Lesser General Public License v3.0
792 stars 182 forks source link

Fixes to `Timer` logic #3488

Closed garth-wells closed 3 weeks ago

garth-wells commented 3 weeks ago

The logic behind the interaction between Timer and TimeLogger was messed up. This attempts to fix it without interface changes.

Improves and fixes tests.

jorgensd commented 3 weeks ago

Shouldn't "stop" imply a "flush" if we want to maintain the timing API?

There isn't a way to "pause" a timer, as "start" resets the accumulated time. Thus stop should imply flush.

garth-wells commented 3 weeks ago

Shouldn't "stop" imply a "flush" if we want to maintain the timing API?

The logic has always been that durations are logged with the Timer is destroyed. flush forces this to happen earlier.

There isn't a way to "pause" a timer, as "start" resets the accumulated time. Thus stop should imply flush.

There was a resume - @schnellerhase removed it because it was untested. I could now be added back (I think it should).

jorgensd commented 3 weeks ago

Shouldn't "stop" imply a "flush" if we want to maintain the timing API?

The logic has always been that durations are logged with the Timer is destroyed. flush forces this to happen earlier.

That is a bit tedious on the python side, as a user would manually have to delete a timer if it is not made with a context manager, or call flush explicitly. This assumption has at least never been documented on the Python-side of things.

I am ok with forcing a flush on the python side, but then that has to be documented as a prerequisite in the C++ and Python documentation of the timing function.

While looking through this, I also observe that the docs are wrong in C++: https://github.com/FEniCS/dolfinx/blob/main/cpp/dolfinx/common/timing.h#L30-L34 as it still talks about a timing triplet.

garth-wells commented 3 weeks ago

The Python wrappers could call stop and flush.

schnellerhase commented 3 weeks ago

There was a resume - @schnellerhase removed it because it was untested. I could now be added back (I think it should).

This also allowed for one less state to manage in the Timer and thus simpler logic. I think keeping the states to a minimum is best here.

What do we need the resume/restart functionality for? Together with the TimeLogger I think we can do without.

jorgensd commented 3 weeks ago

There was a resume - @schnellerhase removed it because it was untested. I could now be added back (I think it should).

This also allowed for one less state to manage in the Timer and thus simpler logic. I think keeping the states to a minimum is best here.

What do we need the resume/restart functionality for? Together with the TimeLogger I think we can do without.

If we use the TimeLogger for resuming timings, then stop should definitely flush, as there is no clear benefit of keeping the object around then?

garth-wells commented 3 weeks ago

There was a resume - @schnellerhase removed it because it was untested. I could now be added back (I think it should).

This also allowed for one less state to manage in the Timer and thus simpler logic. I think keeping the states to a minimum is best here. What do we need the resume/restart functionality for? Together with the TimeLogger I think we can do without.

If we use the TimeLogger for resuming timings, then stop should definitely flush, as there is no clear benefit of keeping the object around then?

There are users who use stop/resume. It wasn't covered by tests (it is now), but that doesn't mean it's not used.

Timings should only be logged to when finished (by the destructor, or when manually flushed), otherwise the logger counter get messed up.