Instrumentation fixes for #176 and #181 #179
Conversation
Reasons: 1) It isnt used anywhere 2) It is buggy (e.g. memory leak) if you were to BeginSession() another session (without EndSession(), or were to BeginSession() from differnet threads.
|
Why does code commited have whitespace inconsistency? |
|
There are some problems with BeginSession() / EndSession() as currently designed.
|
Whoops, sorry. I use spaces. Cherno uses tabs. Should there be a .editorconfig? |
|
Doesn't .editorconfig do nothing? |
|
Either way, we have noted in our contribution guidelines that we use tabs and not spaces. |
I was under the impression that it could be used to control things like indentation style. @Puddlestomper yes, I know. I missed that I'd used spaces. Sorry. |
@WhoseTheNerd Didn't I? Can you give me a hint, because I'm not seeing it. |
|
We should consider leaving the We definitely should fix the memory leak though. We can remove the code that uses the struct (as you did), or just delete the pointer it in the |
|
Yes, thinking about this some more, I agree that InstrumentationSession should be left in there, as it can be used for multiple session support. I will play around with this a bit more, and update the PR. |
Now it is fixed. |
…ession()/EndSession(). There is still a race-condition in BeginSession() on the m_CurrentSession pointer.
|
I think this is good to go now. I have updated the PR description to more accurately describe what I ended up doing. What do others think? |
… under lock in WriteProfile(). Always write an initial comma when writing a proflie result (write an initial empty result in WriteHeader() so that the json is still properly formed)
|
@reductor Thanks, I missed that. Reason why code reviews are good. |
|
Another thought of mine, shouldn't we split into .h and .cpp file as the file grows? |
|
I think there is a possibility this will try to write to an already closed stream. |
… it has lock. (otherwise other thread could potentially EndSession() after WriteProfile() has checked m_HasSession. Removed m_HasSession since it is now pretty much redundant.
|
@Emilundpixeln Good catch, thanks. Need to check that there still is a session after getting the lock in WriteProfile(). After that it seems like the code is a bit cleaner to just check m_CurrentSession and not bother with the atomic m_HasSession at all - so I've stripped that out. It means that if one thread is still profiling when another thread has ended the session, then the still-profiling thread will waste a bit of time formatting json which is then never used. I don't think we should get too hung up on that - its kind of an edge case error situation anyway. |
I've left as-is for now. Hopefully the file wont get any bigger for a while (maybe some day when a better profiling sub system is needed). |
Describe the issue (if no issue has been made)
Some small tidy ups to Instrumentor:
RemovedunusedInstrumentationSession.NotethatInstrumentationSessionwasalsoapotentialmemoryleakifBeginSession()/EndSession()arenotperfectlypaired.PR impact
Fixes #176
Fixes #181
Additional context
There is still a memory leak if an instrumentation session is begun but not ended when app exits.
This could be easily fixed (by making m_CurrentSession into a Hazel::Scope, but it comes at the cost of having to add a constructor for InstrumentationSession (Hazel::CreateScope({name}) will not work)
One could remove InstrumentationSession (this was my first idea), and use m_OutputStream.is_open() in place of checking for non-null m_CurrentSession. However, I've left InstrumentationSession in there as it could come in useful later (for example: If it is desired to create instrumentation sessions that only capture particular categories of profiling output - you will need to track which categories are active in the InstrumentationSession instance)
I have not addressed the concurrent start timestamp issue, as Cherno has said he is going to address it in an upcoming episode, and he probably has his own idea for how to fix it.
(my suggestion would be a thread_local static counter that is incremented/decremented automatically via InstrumentationTimer RAII. Just add this counter on to the timestamp. (yes, it will result in timestamps that are a few ticks off, but a tick here is 0.001 of a millisecond... it shouldn't matter)