well-typed / ghc-events-analyze

BSD 3-Clause "New" or "Revised" License
67 stars 23 forks source link

Why are threads recorded as having terminated at the end of a window? #17

Closed edsko closed 8 years ago

edsko commented 8 years ago

@WillSewell I've merged #16 tentatively, but I'm not 100% sure it's correct. So if I understand your code correctly, if a thread start outside the scope of any window, then we basically don't have a EventAnalysis to add it to, and so you add it to pendingIds and record them as having started at the start of the next window. That makes a certain amount of sense to me.

However, then when a window finishes you record all threads as having terminated. Why?

edsko commented 8 years ago

In particular, shouldn't they still be listed as being running in the next window?

WillSewell commented 8 years ago

Thanks for the feedback. I think we may have different understandings of what windows are. My understanding was that there should only be a single window for one run of a program, and that would determine the period of time that should be profiled/graphed. Are you saying that there can be multiple windows? I'm not sure how these would be graphed.

edsko commented 8 years ago

I didn't add the windows feature myself, but I think the idea is that we can split a long running program into multiple windows, and get separate graphs for those. If you don't enable windowing, you basically get a single window for the entire program run.

edsko commented 8 years ago

(The separate graphs are output to separate files, IIRC).

WillSewell commented 8 years ago

Ah yes I see. It makes sense to me now. I can't implement a fix right now because my dev laptop is broken. Hopefully I will get the chance in the next week or so.

I'll also add some documentation for windows, because I remember having to look in the source code to figure what they were for/how they worked -- and that clearly didn't work out too well!

edsko commented 8 years ago

Yeah, I had the same trouble when I was trying to figure out if your changes made sense :)

Thanks!

WillSewell commented 8 years ago

@edsko, I had a think about this today, and I'm not sure what approach I could take. I could either:

Do you have any thoughts/preferences?

edsko commented 8 years ago

Actually, I like your second solution a lot better. That would seem to fit much better with the original setup of how things are recorded, and then, as you say, the quirks of rendering with multiple windows will be localised in one place.

Either way, I suppose with windows we need to be able to say "this thread was already running when this window started" or "this thread was still running when this window ended", but if we go with your second option than that can be done in the renderer.

(There is a third option, isn't there? If we do have "events" for "still running at window end" or "already running at window start", you could add those "still running" events when closing a window and add the "already running" when opening a window. Then you could stick with the state as it is. That said, I think doing this after the fact is a lot cleaner. The only thing I'm not entirely sure about is whether it will mean we accumulate more stuff in memory, rather than writing windows to disk as they complete. But that might not happen now either, I'm not sure.)

WillSewell commented 8 years ago

Thanks for the feedback. I'm still experimenting with what might work best, but currently I think the first might be the best option. Whatever is done, the EventAnalysis needs to be split into a list of EventAnalysis (or similar) per window for the aggregation functions (quantize, createReport) work. This could be done in a separate step, but I think it would be easier just to write the per window information into a separate data structure per window as the analysis is performed.

edsko commented 8 years ago

That's already done, isn't it? That's why we have a list of them?

WillSewell commented 8 years ago

Yes, but have some global state (to record thread stop and start times) along with the list of ThreadAnalysis.

edsko commented 8 years ago

Fixed in #20.