Add new version of the ContentionStart event #72627

gleocadie · 2022-07-21T18:44:08Z

The goal of this PR is to enrich the information provided in the ContentionStart event.

Today, we have the callstack (attached to the ContentionStart event), the contention duration (with the ContentionEnd event).

By adding the ObjectID of the lock object and the ThreadID of the thread that currently holds the lock, we will be able to build the view which thread is blocking other threads on which lock object.

In this new event, we add the lock object id and the id of the thread that owns it.

dotnet-issue-labeler · 2022-07-21T18:44:14Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

src/coreclr/vm/ClrEtwAll.man

src/coreclr/vm/syncblk.cpp

gleocadie · 2022-07-28T14:02:16Z

src/coreclr/vm/ClrEtwAll.man

@@ -1742,6 +1742,21 @@
 </UserData>
 </template>

+ <template tid="ContentionStart_V2">


I wonder if putting the LockId and OwnerId wouldn't be better on the Stop event, WDYT?

I think it makes more sense on the start event, since you want to know what was holding it that triggered the contention

davmason

The change looks good to me, but @kouvel should take a look before we merge. He is out of the office for a few more days though

brianrob · 2022-08-01T17:57:21Z

src/coreclr/vm/syncblk.cpp

@@ -2549,14 +2554,9 @@ BOOL AwareLock::EnterEpilogHelper(Thread* pCurThread, INT32 timeOut)
 QueryPerformanceCounter(&startTicks);

 // Fire a contention start event for a managed contention
- FireEtwContentionStart_V1(ETW::ContentionLog::ContentionStructs::ManagedContention, GetClrInstanceId());
+ FireEtwContentionStart_V2(ETW::ContentionLog::ContentionStructs::ManagedContention, GetClrInstanceId(), OBJECTREFToObject(obj), m_HoldingThread);


Rather than writing out m_HoldingThread which is the address of the Thread object in memory, can we change this to m_HoldingThread->GetOSThreadId64()? This will allow a profiler to display what the thread that currently holds the lock is doing. With the pointer value, you would need to provide another event that correlates between Thread* and the actual OS thread ID.

Another potential issue here is that the value of obj can change during GC relocation. Is the goal to also watch the GC survival and movement events, or do you want to try and have a more durable ID? I'm wondering if AwareLock should also have a set of creation/destruction events so that you can see the full lifetime of the lock.

@brianrob good point. I'll make the change to replace the pointer by the OS Thread id.

Another potential issue here is that the value of obj can change during GC relocation.

Does it mean that the address is changed also ?
The goal, at least in this PR, is not to watch the lock object lifetime, but more having an ID to do some correlation.

Does it mean that the address is changed also ?

Yes the object address also changes when it's moved. Could pass in this instead to include the AwareLock pointer as just an identifier for the lock.

Using this is a good idea. Worth calling out though that it is possible to destroy and then create a new lock at the same address. To make sure that you know about this, you may wish to write an event in the AwareLock ctor so that you know when the aware lock is created. This type of pattern is other places such as module and type loads. We use the address of the module or type in memory, but handle questions of lifetime using load/unload events.

Thanks for the info.
So if I understand correctly, in the AwareLock ctor I emit an event (we know the lock is created) and I will have to emit an event in the dtor to know when the lock is deleted, right ?

I'll do the change taking into account your comment.

To avoid emitting too many events, I think you can safely emit just the creation event. If you see two creation events at the same address, you know that it's a new lock.

brianrob

A couple of fixups, but overall coming together.

brianrob · 2022-08-05T22:12:27Z

src/coreclr/vm/ClrEtwAll.man

+ <template tid="ContentionStart_V2">
+ <data name="ContentionFlags" inType="win:UInt8" map="ContentionFlagsMap" />
+ <data name="ClrInstanceID" inType="win:UInt16" />
+ <data name="LockObjectID" inType="win:Pointer" />


Recommend calling this LockID, and then match with the same name in the AwareLockCreated event.

brianrob · 2022-08-05T22:15:58Z

src/coreclr/vm/syncblk.cpp

 {
 QueryPerformanceCounter(&startTicks);

 // Fire a contention start event for a managed contention
- FireEtwContentionStart_V1(ETW::ContentionLog::ContentionStructs::ManagedContention, GetClrInstanceId());
+ FireEtwContentionStart_V2(ETW::ContentionLog::ContentionStructs::ManagedContention, GetClrInstanceId(), OBJECTREFToObject(obj), this);


The two new parameters are LockID and OwningThreadID, so the first new parameter should be this, and the second should be pCurThread->GetOSThreadId64().

Oh yeah you are right... I forgot this one. I was focused on the AwareLock event.

I think it should be m_HoldingThread instead of pCurThread, because we want the thread that currently holds the lock

Yes, you're right.

kouvel · 2022-08-05T22:46:12Z

src/coreclr/vm/ClrEtwAll.man

@@ -3652,6 +3685,12 @@
 task="Contention"
 symbol="ContentionStop_V1" message="$(string.RuntimePublisher.ContentionStop_V1EventMessage)"/>

+ <event value="414" version="0" level="win:Informational" template="AwareLockCreated"


Looks like event ID 90 is not used:

Suggested change

<event value="414" version="0" level="win:Informational" template="AwareLockCreated"

<event value="90" version="0" level="win:Informational" template="AwareLockCreated"

kouvel · 2022-08-05T23:01:10Z

src/coreclr/vm/ClrEtwAll.man

@@ -211,6 +211,7 @@
 value="8" eventGUID="{561410f5-a138-4ab3-945e-516483cddfbc}"
 message="$(string.RuntimePublisher.ContentionTaskMessage)">
 <opcodes>
+ <opcode name="AwareLockCreated" message="$(string.RuntimePublisher.AwareLockCreatedOpcodeMessage)" symbol="CLR_AWARELOCK_CREATED_OPCODE" value="10"> </opcode>


Maybe a new opcode is not necessary, the win:Start opcode can be used instead

I think win:Info is a good choice here since we are not starting an action, and there won't be a corresponding stop.

kouvel · 2022-08-05T23:06:37Z

src/coreclr/vm/ClrEtwAll.man

@@ -3652,6 +3685,12 @@
 task="Contention"
 symbol="ContentionStop_V1" message="$(string.RuntimePublisher.ContentionStop_V1EventMessage)"/>

+ <event value="414" version="0" level="win:Informational" template="AwareLockCreated"


The name AwareLock is specific to the current implementation. The implementation may be changed, or another implementation may be added in the future (eg. #34812) and may want to reuse the same events, maybe just LockCreated would be more generic

Agreed, this is a good idea.

tommcdon · 2022-08-11T23:42:06Z

@kouvel @brianrob @davmason can this PR be merged?

brianrob · 2022-08-11T23:56:33Z

Not yet. This is waiting for the most recent round of code review feedback to be addressed.

tommcdon · 2022-08-12T14:59:02Z

Thanks for moving the milestone to 8.0 @kouvel. Given that this PR is still under iteration moving to 8.0 is correct.

brianrob

LGTM. @kouvel, what do you think?

brianrob · 2022-08-18T19:36:23Z

@davmason, it looks like the test failures are related to the new LockCreated event, which is triggering an assert. I suspect this is because the assumptions made in EventPipe code are not met at this call site. This will probably require some guidance to determine the right fix, though I'm not familiar enough with how this code has evolved to know exactly what to recommend - possibly moving where the event is emitted.

davmason · 2022-08-23T07:59:41Z

@davmason, it looks like the test failures are related to the new LockCreated event, which is triggering an assert. I suspect this is because the assumptions made in EventPipe code are not met at this call site. This will probably require some guidance to determine the right fix, though I'm not familiar enough with how this code has evolved to know exactly what to recommend - possibly moving where the event is emitted.

Looking at it, it should be possible to declare that it is legal to take them in that order and everything should work. Our crst code tries to make everything illegal unless specifically whitelisted.

@gleocadie can you do the following to update it:

Rebase your changes against the latest in master since there are more recent changes to crsttypes.def than your branch contains

Add SyncBlockCache to the AcquiredAfter list for Crst CodeVersioning here:

runtime/src/coreclr/inc/CrstTypes.def

Lines 396 to 400 in 40749f6

 Crst CodeVersioning 

 AcquiredBefore LoaderHeap SingleUseLock DeadlockDetection JumpStubCache DebuggerController FuncPtrStubs 

 AcquiredAfter ReJITGlobalRequest ThreadStore GlobalStrLiteralMap SystemDomain DebuggerMutex MethodDescBackpatchInfoTracker 

 ReadyToRunEntryPointToMethodDescMap ClassInit AppDomainCache TypeIDMap FusionAppCtx 

 End

Regenerate crsttypes.h using the tool from here: https://github.com/dotnet/runtime/blob/main/src/coreclr/inc/CrstTypeTool.cs
- I find it easiest to create a new project with dotnet new console, add the code from crsttypetool.cs to program.cs and then copy CrstTypes.def over to that folder, then run with dotnet run. It will generate crsttypes.h and you can copy that over in to your enlistment, then check it in.

kouvel · 2022-08-23T13:22:13Z

There are two failures, for the second failure it looks like the place where the LockCreated event is emitted needs to change, which would also make the above lock ordering unnecessary. I also found a separate minor issue. I'll go ahead and post my fixes.

kouvel · 2022-08-23T14:00:22Z

Changes in the latest commit:

Moved firing the LockCreated event to outside the FAULT_FORBID region and outside the SyncBlockCache lock
m_HoldingThread may be null. Also, it can't be dereferenced since after reading it the holding thread may release the lock, exit, and the Thread object may be deleted. Changed to pass in the Thread pointer in m_HoldingThread into the event and changed the event to take a pointer for the thread ID. There are already ThreadCreating and ThreadRunning events that include the native Thread pointer.
Undid some code reordering in AwareLock::EnterEpilogHelper since the managed object ref is not used for the event anymore. Also some miscellaneous changes.

kouvel · 2022-08-24T18:43:23Z

@brianrob and @davmason, if the latest commit looks ok to you, I think we can merge

brianrob

Looking good. A couple of small things.

brianrob · 2022-08-24T19:21:53Z

src/coreclr/vm/syncblk.cpp

 }

+ // Fire the LockCreated event last, such that it is outside the FAULT_FORBID region above and outside the SyncBlockCache
+ // lock. The eventing system may need to allocate.
+ syncBlock->FireLockCreatedEvent();


Does this guarantee that we only fire this event when a lock is created? There are other paths that create syncblocks, right?

I didn't see any other paths that create sync blocks

From what I can see, there are others. Here's an example: https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/object.cpp#L987

It looks like this path also goes through GetSyncBlock(), which is modified here, so it would also emit the event

Right. There are others too, so I don't think this is the right place to emit the event, as it will be potentially much more verbose (and duplicative) than we want.

I thought the sync block would be created once per object and no more? I

That's only true for precious syncblocks. Syncblocks that are not marked as precious can be reclaimed and re-created multiple times.

That's only true for precious syncblocks. Syncblocks that are not marked as precious can be reclaimed and re-created multiple times.

Wouldn't that mean that each time a sync block is recreated for an object, that it may have a different pointer to the AwareLock and hence warrant emitting the event?

Yes, that's correct.

I think that Brian's concern is that we will emit this event for all SyncBlocks even the ones that are not ever used for locks. SyncBlocks are created for number of different reasons (the name "syncblock" is misleading). For example, interop creates them a lot for COM objects and marshalled delegates.

Yes. What do you think about having the AwareLock log that it was created lazily - on first acquisition?

I see. Yes, it could probably be emitted lazily. Taking a step back, is the lock ID really necessary? The stack trace of the contention event would probably be sufficient to identify the lock. I figure the LockCreated event's stack trace would provide similar information. A benefit may be to have only a few LockCreated events and avoid taking stack traces for contention events, but at the cost of missing those created events in some cases. Is there any other benefit?

brianrob · 2022-08-24T19:23:39Z

src/coreclr/vm/syncblk.cpp

+ ETW::ContentionLog::ContentionStructs::ManagedContention,
+ GetClrInstanceId(),
+ this,
+ m_HoldingThread);


The last argument here should actually be the OS thread ID. This allows users of the event to match the lock up with the thread that is holding it in a trace. The memory address of the lock won't allow for this.

I commented on this in #72627 (comment). It looks like the holding thread pointer cannot be dereferenced here because there's not guarantee that it would be alive. The ThreadRunning event is emitted from a newly started thread and can be used to map the Thread pointer to the OS thread ID.

For threads that are created from outside the runtime and then enter the runtime, I believe the ThreadCreated event can be used to do the same mapping.

I don't think we can depend on the ThreadRunning event because there is nothing that guarantees that this event is present in the trace. If the thread already exists when the trace is started, then we have no way to correlate it back to the OS thread. That said, this call is made in a function that requires cooperative mode. Given this, even if the thread dies, the memory shouldn't be reclaimed until after the next GC when the managed object Finalizer executes, right?

I don't think we can depend on the ThreadRunning event because there is nothing that guarantees that this event is present in the trace. If the thread already exists when the trace is started, then we have no way to correlate it back to the OS thread.

That's also true for the new LockCreated event. It seems like the ordering of those events would matter, so having a rundown of thread and lock objects may not help much to identify the thread. Perhaps the thread object is stored as the holding thread because it is easily accessible and is the minimum work necessary to identify the holding thread. Storing the OS thread ID instead would involve an indirection, and OS thread IDs may also be reused at some point, I suspect this kind of issue would exist in multiple places.

That said, this call is made in a function that requires cooperative mode. Given this, even if the thread dies, the memory shouldn't be reclaimed until after the next GC when the managed object Finalizer executes, right?

I'm not sure about that, coop GC mode would ensure that the managed thread object would be alive if accessible, but once the managed thread object is deemed unreachable and the finalizer runs, perhaps the native thread object can be deleted irrespective of coop GC mode. Maybe there are further constraints that I'm not aware of?

emitting the LockCreated event more often than we should (e.g. every time there is a call to GetSyncBlock)

Why would the LockCreated event be emitted every time GetSyncBlock() is called?

With regard to the OS thread ID. You're right that these can be re-used. This is addressed because the OS will tell us when a thread is created/destroyed. Thus, if there is a re-use within a trace, we'd see this from the OS thread events.

Why would there not be a similar problem to what you mentioned before regarding missing events when starting a trace on an already-running process? For instance, the create/destroy OS events for threads may not be captured by the trace.

A way to address this would be for the thread that acquires the lock to save its thread ID into the lock when tracing is on and clear it when the lock is relinquished. What do you think of that?

The information stored is probably minimal, in the sense what is stored is what is most easily accessible for correct behavior, a different implementation may store something else and that should be ok. As I mentioned before, storing the thread ID would involve an indirection. Changing the implementation for this particular reason is probably something that would need a lot more scrutiny, I don't see enough reason for it yet.

Why would the LockCreated event be emitted every time GetSyncBlock() is called?

You're right, it shouldn't. I missed the fact that the cache lookup occurs at the beginning of this function, and returns early.

Why would there not be a similar problem to what you mentioned before regarding missing events when starting a trace on an already-running process? For instance, the create/destroy OS events for threads may not be captured by the trace.

You technically don't need to see the thread creation event to know that it existed at the beginning of the trace, as long as you see samples or events on the thread. What you do need is to have a thread creation event during the trace to know that the thread ID has been re-used. That said, the OS provides thread/process rundown events at the beginning of the trace to tell profilers about the existence of all processes and traces.

The information stored is probably minimal, in the sense what is stored is what is most easily accessible for correct behavior, a different implementation may store something else and that should be ok. As I mentioned before, storing the thread ID would involve an indirection. Changing the implementation for this particular reason is probably something that would need a lot more scrutiny, I don't see enough reason for it yet.

I'm not sure I follow. Can you tell me a little bit more here? Are you saying that you don't think having the OS Thread ID is worthwhile here?

That said, the OS provides thread/process rundown events at the beginning of the trace to tell profilers about the existence of all processes and traces.

Curious, does this work with circular buffers? More generally, is there some mechanism by which a trace taken with a circular buffer, where the buffer fills up and is overwritten, can still have an accurate snapshot of live objects through events at the beginning of the trace?

Are you saying that you don't think having the OS Thread ID is worthwhile here?

No, mapping to a thread ID would probably be the most useful part. I don't think we should change or add to what is stored for the lock-holding thread. Perhaps taking the thread store lock could enable emitting the OS thread ID.

A thread that orphans a lock does not clear the holding thread pointer before its Thread object is destroyed, so acquiring the thread store lock still would not make it feasible to dereference the holding thread pointer. There seem to be a few other related issues and fixing them properly would likely be more involved. For now, we could also store the OS thread ID when the event keyword is enabled as you suggested and update it after the other issues are fixed.

Updated in the latest commit to store and send the OS thread ID, since I think it would be possible to continue doing so after the complete fixes. Still curious about the question in #72627 (comment), as I've run into that issue before and a solution to it may be useful elsewhere.

kouvel · 2022-08-29T21:57:39Z

Updated in the latest commit to store and send the OS thread ID. I think the main remaining question is #72627 (comment). The LockCreated event wouldn't help to identify the managed object used for locking, I'm also not sure how useful it would be to identify the managed object, and I don't see much additional benefit in having a LockID.

tommcdon · 2022-09-26T13:45:32Z

@kouvel @brianrob do you have additional thoughts on a path forward for this PR? Do we need to actually consume these changes in tooling to prove/disprove the usefulness for lock contention investigations?

brianrob · 2022-09-26T23:45:26Z

Sorry, this got lost in my inbox. I think that the latest change @kouvel added is great.

As for the remaining question, I am not quite sure how to address this. Let's say that we don't have a creation event. We will never know how many locks we're dealing with. For example, let's say that there is a type of object that gets processed, and some of the work that happens must happen under lock. The processing is likely to happen with the same stack over and over, but there may be a particular instance or instances that have more contention than others just based on the data. Given that sync blocks can be re-used, it would be quite difficult for us to know how many locks are in play here - we'd have the LockID that would help, but as those IDs are re-used over time, we would not be able to tell. This is a fairly classic pattern that we see across the runtime - other examples of this type of thing include assembly loads being assigned address space, jitted code for collectible assemblies, and GC heap survival events to report object movement during compaction.

I think that if we don't have a LockID or LockCreated event, and just try to depend upon the stack, the value of the additional information decays fairly heavily because you can no longer determine lock lifetime or which stacks are acquiring which lock.

kouvel · 2022-09-27T00:07:40Z

Having the AwareLock pointer available as the LockID would enable determining how many and which of the AwareLocks are contending a lot, but it may difficult if not impossible to identify from a trace which managed objects the heavily contending locks are associated with. Perhaps the info could be used as a trigger for heap dump collection for further investigation or something like that for cases where more info than the stack trace is necessary.

jkotas · 2022-09-27T00:12:59Z

Should the lock created event have associatedObjectID parameter that can be used to identify the primary object instance that the lock is associated with?

brianrob · 2022-09-27T00:15:01Z

Just paging this back into my memory, so let me know if I miss something or state the opposite of what I was wanting earlier. I think using the AwareLock pointer is fine to identify the lock. The key is that the pointer can be re-used, and so we won't know if it's been re-used unless we emit the LockCreated event.

Should the lock created event have associatedObjectID parameter that can be used to identify the primary object instance that the lock is associated with?

If this is easily available, I think it's a great idea. For long-running processes, capturing a dump and a trace together could provide some great insight into contention patterns.

kouvel · 2022-09-27T00:20:55Z

I guess having both the AwareLock pointer and the managed object pointer could be more useful. Initially the managed object pointer was being sent for the LockID, having the AwareLock makes it easier to tell how many locks are contending heavily.

brianrob · 2022-09-27T00:24:07Z

The one thing we'll need to be aware of is that the objectID can change and so we may need to use something like the heap survival and movement events to make sure that we get the right object id. That said, I don't think we should spec that out completely just yet.

- Depends on dotnet/runtime#72627

kouvel · 2022-10-19T23:13:45Z

Updated and added PerfView changes in microsoft/perfview#1731

tommcdon · 2022-10-20T18:21:19Z

Thank you @kouvel @brianrob @jkotas! Are there any other changes needed to merge this PR?

jkotas

LGTM

Add new version of the ContentionStart event

1988694

In this new event, we add the lock object id and the id of the thread that owns it.

ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 21, 2022

davmason requested a review from kouvel July 21, 2022 19:00

davmason assigned gleocadie Jul 21, 2022

davmason added the area-Tracing-coreclr label Jul 21, 2022

davmason added this to the 7.0.0 milestone Jul 21, 2022

davmason requested changes Jul 21, 2022

View reviewed changes

src/coreclr/vm/ClrEtwAll.man Outdated Show resolved Hide resolved

src/coreclr/vm/syncblk.cpp Outdated Show resolved Hide resolved

ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 21, 2022

Change type of LockOjectID and LockOwnerThreadID

5fb79fd

ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 21, 2022

gleocadie requested a review from davmason July 21, 2022 21:59

gleocadie commented Jul 28, 2022

View reviewed changes

gleocadie mentioned this pull request Jul 29, 2022

Update the doc with the new version of ContentionStart dotnet/docs#30412

Merged

davmason approved these changes Jul 29, 2022

View reviewed changes

gleocadie mentioned this pull request Aug 1, 2022

Add support for new version of ContentionStart event microsoft/perfview#1675

Closed

brianrob reviewed Aug 1, 2022

View reviewed changes

Add AwareLockCreated event

d6d6398

gleocadie force-pushed the gleocadie/add-new-version-of-contention-event branch from c0b7d3c to d6d6398 Compare August 5, 2022 21:49

Create stack for AwaredLockCreated event

d214618

brianrob reviewed Aug 5, 2022

View reviewed changes

Address PR comment

4550dc5

kouvel reviewed Aug 5, 2022

View reviewed changes

This was referenced Aug 8, 2022

Infra improvements for Helix #68176

Closed

GC/API/GC/GetGCMemoryInfo/GetGCMemoryInfo.sh test failing intermittently on CoreCLR Linux ARM32 #73247

Closed

kouvel modified the milestones: 7.0.0, 8.0.0 Aug 11, 2022

Address PR comment

1559e3d

brianrob approved these changes Aug 18, 2022

View reviewed changes

Fix a couple of issues

ba3cca1

kouvel approved these changes Aug 24, 2022

View reviewed changes

brianrob reviewed Aug 24, 2022

View reviewed changes

Store and send the lock-holding OS thread ID

292656e

Emit lock created event lazily, include managed object in events

4d529a5

kouvel added a commit to kouvel/perfview that referenced this pull request Oct 19, 2022

Add ContentionStart_V2 and LockCreated events

f3037d8

- Depends on dotnet/runtime#72627

kouvel mentioned this pull request Oct 19, 2022

Add ContentionStart_V2 and LockCreated events microsoft/perfview#1731

Merged

This was referenced Oct 20, 2022

Tracking issue for CI build timeouts #76454

Closed

Nuget restore error caused by 503s dotnet/arcade#11239

Closed

jkotas approved these changes Oct 20, 2022

View reviewed changes

kouvel merged commit 69a1da6 into dotnet:main Oct 20, 2022

ghost locked as resolved and limited conversation to collaborators Nov 20, 2022

	<event value="414" version="0" level="win:Informational" template="AwareLockCreated"
	<event value="90" version="0" level="win:Informational" template="AwareLockCreated"

Add new version of the ContentionStart event #72627

Add new version of the ContentionStart event #72627

Conversation

gleocadie commented Jul 21, 2022

dotnet-issue-labeler bot commented Jul 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davmason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kouvel Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianrob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tommcdon commented Aug 11, 2022

brianrob commented Aug 11, 2022

tommcdon commented Aug 12, 2022

brianrob left a comment

Choose a reason for hiding this comment

brianrob commented Aug 18, 2022

davmason commented Aug 23, 2022

kouvel commented Aug 23, 2022

kouvel commented Aug 23, 2022

kouvel commented Aug 24, 2022

brianrob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kouvel commented Aug 29, 2022 • edited Loading

tommcdon commented Sep 26, 2022

brianrob commented Sep 26, 2022

kouvel commented Sep 27, 2022

jkotas commented Sep 27, 2022

brianrob commented Sep 27, 2022

kouvel commented Sep 27, 2022

brianrob commented Sep 27, 2022

kouvel commented Oct 19, 2022

tommcdon commented Oct 20, 2022

jkotas left a comment

Choose a reason for hiding this comment

kouvel Aug 2, 2022 •

edited

Loading

kouvel commented Aug 29, 2022 •

edited

Loading