Editorial: Various changes around execution contexts #2962

jmdyck · 2022-11-22T04:48:42Z

This PR follows up on some things that came up in the discussion of PR #2665, and then goes farther.

I've organized it into a bunch of fairly focused commits, in case that helps with review. The commits are divided into 3 groups.

(Commits marked with "(see commit msg)" have some extra discussion in the commit message.)

Transfer control:
- Replace the 6 "Resume the suspended evaluation" steps (next-side).
- Replace the 3 remaining blocking-Resume steps (yield-side).
- Refactor GeneratorStart's sub-algorithm so that it has only one Return step.
- Replace 3 Return steps (yield-side). (see commit msg.)
- Re-word some steps that talk about resuming execution contexts.
- Define "transfer control".
Resume+Suspend:
(I think these resolve Clarify Running Execution context and the impact of push/pop and suspend/resume #2409.)
- Eliminate Resume steps. (see commit msg)
- Eliminate Suspend steps. (see commit msg)
Execution Context section:
- Move a paragraph
- Make the EC Stack subsection (see commit msg)

One of the side-effects is to treat execution contexts more like just data structures. I.e., an EC isn't an active thing, it doesn't execute code, and suspending/resuming it makes no more sense than suspending/resuming a List or Record. If you like that direction, I could do some more cleanup. (I don't think there's much left.)

See also PR #2246 and PR #2681, which are independent of this PR but in the same conceptual neighborhood.

jmdyck · 2022-11-22T22:58:53Z

(force-pushed to fix an 'unused-declaration' error from ecmarkup)

jmdyck · 2023-01-14T01:46:44Z

Force-pushed to resolve merge-conflicts from PR #2681.

Also, I've added a fixup commit that makes 'code evaluation state' a bit more concrete.

michaelficarra · 2023-01-25T17:47:42Z

spec.html

+    </ul>
+    <p>The suspended execution associated with _aContext_ will stay suspended until some step transfers control back to _aContext_, which might never happen.</p>
+    <emu-note>This is analogous to the concept of coroutines in programming languages.</emu-note>
+    <emu-note>In practice, _aContext_ was the running execution context until just before the “transfer control” step, and _bContext_ is the new running execution context. However, the “transfer control” step itself doesn't manipulate the execution context stack.</emu-note>


Why is this note needed? Who does this clarification help, and what does it help them with?

I think it helps the person who reads the definition of "transfer of control" and thinks "But wait, if you transfer control to an execution context, does that make it the running execution context? Does 'transfer control' also manipulate the execution context stack?" (Similar to issue #2409. Not that exact question, but a similar uncertainty about implicit stack manipulation.)

Now, in the absence of that note, I think you could still answer those questions by going to the places where 'transfer of control' occurs, and seeing what happens to the EC stack in neighboring steps. But I think it's useful to have a note to this effect close to the definition of 'transfer of control'.

michaelficarra · 2023-01-25T17:50:53Z

spec.html

+      <h1>Execution Context Stack</h1>
+      <p>An agent's <dfn id="execution-context-stack" variants="execution context stacks">execution context stack</dfn> is used to organize some or all of the agent's execution contexts. The running execution context is always the top element of this stack.</p>
+      <p>Adding and removing always happens at the “top” of the execution context stack. When an execution context is added, it becomes the topmost (i.e., the running execution context), and when it is later removed, the execution context “below” it becomes topmost again.</p>
+      <emu-note>In the absense of generators, every execution context that is pushed onto the stack is new, and when it's removed from the stack it can be discarded. With generators, some execution contexts outlive their time on the stack, exist for a while outside the stack, and then later are pushed onto the stack again.</emu-note>


I love this note.

Though it should be expanded to include async functions.

For symmetry, should I also change "generators" to "generator functions"? E.g. "In the absence of generator functions and async functions, ..."

Eh, I'm fine with the asymmetry (since there actually is a distinction between "generator" vs "generator function", although not one which is relevant here), and I think it reads better without the extra word.

michaelficarra · 2023-01-25T17:55:11Z

spec.html

+        1. Remove _scriptContext_ from the execution context stack and restore the execution context that is at the top of the execution context stack as the running execution context.
        1. Assert: The execution context stack is not empty.


Suggested change

1. Remove _scriptContext_ from the execution context stack and restore the execution context that is at the top of the execution context stack as the running execution context.

1. Assert: The execution context stack is not empty.

1. Remove _scriptContext_ from the execution context stack.

1. Assert: The execution context stack is not empty.

1. NOTE: The execution context that is at the top of the execution context stack is now the running execution context.

We don't make something the running execution context. By the definition of the term, it just is because of its position on the top of the stack.

I think your point is that the wording "and restore ... as the running execution context" misleadingly suggests the need to do something (extra) to make that happen. I basically agree.

However, the "Remove ... and restore ..." wording is well-established in the status quo, in lots of places that this PR doesn't touch, so fixing it should probably be the job of a different PR.

[Later: I made it the first commit of this PR.]

michaelficarra · 2023-01-25T17:57:17Z

Do we need any wording about what it means for "the running execution context" if the execution context stack is somehow empty? Do we need to describe extra invariants about the non-emptiness of the execution context stack?

spec.html

jmdyck · 2023-01-25T22:01:14Z

Hm, maybe the 2 commits for "Execution Context section" should be in a separate PR.

jmdyck · 2023-02-13T16:33:55Z

One of the side-effects is to treat execution contexts more like just data structures. I.e., an EC isn't an active thing, it doesn't execute code, and suspending/resuming it makes no more sense than suspending/resuming a List or Record. If you like that direction, I could do some more cleanup. (I don't think there's much left.)

I've added a commit that eliminates wording that treats execution contexts as 'active' things. Specifically, it eliminates phrasing such as:

an EC tracks the evaluation/execution of code
an EC evaluates/executes code
an EC can wait for an event
code is 'within' an EC
code is evaluated within an EC
control enters an EC

For example, 19 The Global Object says that the global object "is created before control enters any execution context". But the spec doesn't say what it means for control to enter an EC, or when that first happens. (It doesn't use the phrase anywhere else.) And if you (reasonably) guess that it happens when you create an EC and push it onto the stack, that conflicts with InitializeHostDefinedRealm: the global object is created after an EC is pushed onto the stack. So rather than talk about control entering an EC, it seems way more helpful to just say that the global object is created in InitializeHostDefinedRealm.

For another example, 9.9 Forward Progress says that "An agent becomes 'blocked' when its running execution context waits [...] for an external event." But if you look at Atomics.wait and SuspendAgent, there's no mention of anything happening to an execution context, so it seems unnecessary to bring that into the definition of 'blocked'.

jmdyck · 2023-02-13T16:47:45Z

(I didn't eliminate the phrase 'running execution context', despite the fact that it suggests an execution context can 'run'. If we wanted to avoid that, we could call it something like the 'current execution context' or the 'top execution context'. But that would be a lot more disruption for not much benefit, I think.)

jmdyck · 2023-03-01T01:42:53Z

(force-pushed to resolve conflicts from PR #3016)

jmdyck · 2023-03-18T14:39:20Z

(force-pushed to resolve merge conflict)

jmdyck · 2023-03-22T00:46:31Z

(force-pushed to resolve conflict from #3031)

jmdyck · 2023-04-20T22:10:47Z

(force-pushed to resolve merge conflicts)

michaelficarra · 2023-08-30T00:27:37Z

spec.html

        1. Push _scriptContext_ onto the execution context stack; _scriptContext_ is now the running execution context.
        1. Let _script_ be _scriptRecord_.[[ECMAScriptCode]].
        1. Let _result_ be Completion(GlobalDeclarationInstantiation(_script_, _globalEnv_)).
        1. If _result_.[[Type]] is ~normal~, then
          1. Set _result_ to Completion(Evaluation of _script_).
          1. If _result_.[[Type]] is ~normal~ and _result_.[[Value]] is ~empty~, then
            1. Set _result_ to NormalCompletion(*undefined*).
-        1. Suspend _scriptContext_ and remove it from the execution context stack.
+        1. Remove _scriptContext_ from the execution context stack and restore the execution context that is at the top of the execution context stack as the running execution context.


This still sounds like we are requiring some action be taken.

Suggested change

1. Remove _scriptContext_ from the execution context stack and restore the execution context that is at the top of the execution context stack as the running execution context.

1. Remove _scriptContext_ from the execution context stack. The execution context that is now at the top of the execution context stack is the running execution context.

We could also just drop that second sentence.

Okay, I've prepended a commit that eliminates the phrasing "restore [context] as the running execution context".

michaelficarra · 2023-08-30T00:28:54Z

spec.html

+              1. Let _finalCompletion_ be _result_.
+            1. Let _callerContext_ be the running execution context.
+            1. Transfer control from _acGenContext_ to _callerContext_, passing _finalCompletion_.
+            1. NOTE: The above step never completes, because control is never transferred back to _acGenContext_.


Suggested change

1. NOTE: The above step never completes, because control is never transferred back to _acGenContext_.

1. NOTE: This step is never reached because control is never transferred back to _acGenContext_.

I don't want to have to think about what it means for a step to complete.

michaelficarra · 2023-08-30T00:29:46Z

spec.html

-          1. <emu-meta effects="user-code">Resume the suspended evaluation of _genContext_</emu-meta> using NormalCompletion(_value_) as the result of the operation that suspended it. Let _result_ be the value returned by the resumed computation.
-          1. Assert: When we return here, _genContext_ has already been removed from the execution context stack and _methodContext_ is the currently running execution context.
+          1. Transfer control from _methodContext_ to _genContext_, passing NormalCompletion(_value_).
+          1. NOTE: The above step completes only if/when control is transferred back to _methodContext_.


Suggested change

1. NOTE: The above step completes only if/when control is transferred back to _methodContext_.

1. NOTE: This step is reached only if/when control is transferred back to _methodContext_.

jmdyck · 2024-01-11T01:22:37Z

(force-pushed to resolve merge conflicts)

jmdyck · 2024-08-10T14:49:44Z

(force-pushed to resolve merge conflicts)

…ext" Specifically, change: > Remove _X_ from the execution context stack and restore the execution context that is at the top of the execution context stack as the running execution context. to just: > Remove _X_ from the execution context stack. and change: > Remove _X_ from the execution context stack and restore _Y_ as the running execution context. to: > Remove _X_ from the execution context stack. > Assert: _Y_ is the running execution context again. The "restore" phrasing suggests that the implementation needs to do something, when in fact the context in question becomes the running execution context automatically, because the running execution context is always the top element of the execution context stack.

…t it has only one Return step.

In GeneratorStart, AsyncGeneratorStart, and AsyncBlockStart, the Abstract Closure ends with a `Return`. This is odd, because where would it return to? Normally, a `Return` would return to the operation that invoked the Abstract Closure, but in this case, the invocation mechanism is unusual, and in general, the operation that caused it is no longer waiting for a result. Instead, the `Return` is presumably supposed to send control back to the latest next-side `Transfer control` step. So make that explicit.

…on contexts

The only remaining `Resume` steps are the 3 non-blocking Resumes in ScriptEvaluation, ExecuteModule, and PerformEval. Since there's no definition for "resuming" an execution context, and since it doesn't come up any other time that an EC is removed from the EC stack, I'm concluding that it has no normative effect. So remove the remaining `Resume` steps.

As with `Resume`, there's no definition for "suspending" an execution context. And if `Resume` has no normative effect, it seems very unlikely that `Suspend` would. So drop mentions of suspending an execution context. ---- The "Execution Contexts" section does have a paragraph that talks about suspending execution contexts, but it doesn't really say what the effect of `Suspend` is, it just implies that it's a prerequisite for making a different EC become the running EC. But that seems like a pointless hoop to jump through. Whatever meaning you might ascribe to suspending an execution context, you could just as easily imagine that it happens automatically whenever the running execution context changes. There's no need for the spec to call it out explicitly. It's just a source of "bugs", where we fail to say it when we "should".

Take a paragraph that talks about LexicalEnvironment and VariableEnvironment, and move it to a better spot.

Collect stuff about the execution context stack, move it to a new subsection, and then rework it. Note that there a couple of problems in the status quo: "A new execution context is created whenever control is transferred from the executable code associated with the currently running execution context to executable code that is not associated with that execution context." That became false when generators were added. "Transition of the running execution context status among execution contexts usually occurs in stack-like last-in/first-out manner. However, some ECMAScript features require non-LIFO transitions of the running execution context." No, it's always stack-like.

Formerly, we would say: Set the code evaluation state of _genContext_ such that the next time there is a transfer of control to that execution context, _closure_ will be called with no arguments. I.e., we don't say what to set it *to*, just what the eventual effect of the setting should be. This isn't how the spec usually talks. Instead, simply set the code evaluation state to _closure_, and define "transfer of control" to handle that appropriately.

Specifically, eliminate wording such as: - an EC tracks the evaluation/execution of code - an EC evaluates/executes code - an EC can wait for an event - code is 'within' an EC - code is evaluated within an EC - control enters an EC

…ached"

michaelficarra · 2024-10-23T03:02:27Z

FYI https://github.com/tc39/ecma262/pull/3391/files#diff-181371b08d71216599b0acccbaabd03c306da6de142ea6275c2135810999805aR28635-R28636 adds new text that will need updating in this PR.

jmdyck · 2024-10-23T12:59:20Z

Yeah, if #3391 merges first, then this branch will get syntax errors after the rebase.

michaelficarra · 2024-10-24T18:33:48Z

spec.html

          </td>
        </tr>
      </table>
    </emu-table>
-    <p>In most situations only the running execution context (the top of the execution context stack) is directly manipulated by algorithms within this specification. Hence when the terms “LexicalEnvironment”, and “VariableEnvironment” are used without qualification they are in reference to those components of the running execution context.</p>
+    <p>To express the semantics of generators, this document uses steps of the form “<dfn id="transfer-control" variants="transfers control,transfer of control">Transfer control</dfn> from _aContext_ to _bContext_, passing _completion_”, where _aContext_ and _bContext_ are execution contexts and _completion_ is a Completion Record. This means:</p>


Suggested change

To express the semantics of generators, this document uses steps of the form “<dfn id="transfer-control" variants="transfers control,transfer of control">Transfer control</dfn> from _aContext_ to _bContext_, passing _completion_”, where _aContext_ and _bContext_ are execution contexts and _completion_ is a Completion Record. This means:

To express the semantics of generators, this document uses steps of the form “<dfn id="transfer-control" variants="transfer control,transfers control,transfer of control">Transfer control</dfn> from _aContext_ to _bContext_, passing _completion_”, where _aContext_ and _bContext_ are execution contexts and _completion_ is a Completion Record. This means:

For "If ..., transfer control ...".

michaelficarra · 2024-10-24T18:50:15Z

spec.html

-    <p>In most situations only the running execution context (the top of the execution context stack) is directly manipulated by algorithms within this specification. Hence when the terms “LexicalEnvironment”, and “VariableEnvironment” are used without qualification they are in reference to those components of the running execution context.</p>
+    <p>To express the semantics of generators, this document uses steps of the form “<dfn id="transfer-control" variants="transfers control,transfer of control">Transfer control</dfn> from _aContext_ to _bContext_, passing _completion_”, where _aContext_ and _bContext_ are execution contexts and _completion_ is a Completion Record. This means:</p>
+    <ul>
+      <li>Execution of the current algorithm (the one with the “Transfer control” step) is suspended, and a representation of that suspended execution is saved in the code evaluation state of _aContext_.</li>


Suggested change

<li>Execution of the current algorithm (the one with the “Transfer control” step) is suspended, and a representation of that suspended execution is saved in the code evaluation state of _aContext_.</li>

<li>Execution of the current algorithm (the one with the “transfer control” step) is suspended, and a representation of that suspended execution is saved in the code evaluation state of _aContext_.</li>

Consistent with the below reference.

michaelficarra · 2024-10-24T18:53:46Z

spec.html

+    </ul>
+    <p>The suspended execution associated with _aContext_ will stay suspended until some step transfers control back to _aContext_, which might never happen.</p>
+    <emu-note>This is analogous to the concept of coroutines in programming languages.</emu-note>
+    <emu-note>In practice, _aContext_ was the running execution context until just before the “transfer control” step, and _bContext_ is the new running execution context. However, the “transfer control” step itself doesn't manipulate the execution context stack.</emu-note>


Why is this note needed?

😆 I just noticed that I asked this same thing in my original review: #2962 (comment)

michaelficarra · 2024-10-24T19:24:54Z

spec.html

-          1. Resume _callerContext_ passing NormalCompletion(_iteratorResult_). If _genContext_ is ever resumed again, let _resumptionValue_ be the Completion Record with which it is resumed.
-          1. Assert: If control reaches here, then _genContext_ is the running execution context again.
+          1. Transfer control from _genContext_ to _callerContext_, passing NormalCompletion(_iteratorResult_).
+          1. NOTE: This step is reached only if/when control is transferred back to _genContext_.


I like the wording you used above.

Suggested change

1. NOTE: This step is reached only if/when control is transferred back to _genContext_.

1. NOTE: This step is reached only when control is transferred back to _genContext_, which might not happen.

michaelficarra · 2024-10-24T19:29:20Z

spec.html

-    <p>An <dfn variants="execution contexts">execution context</dfn> is a specification device that is used to track the runtime evaluation of code by an ECMAScript implementation. At any point in time, there is at most one execution context per agent that is actually executing code. This is known as the agent's <dfn id="running-execution-context" variants="running execution contexts">running execution context</dfn>. All references to the running execution context in this specification denote the running execution context of the surrounding agent.</p>
-    <p>The <dfn id="execution-context-stack" variants="execution context stacks">execution context stack</dfn> is used to track execution contexts. The running execution context is always the top element of this stack. A new execution context is created whenever control is transferred from the executable code associated with the currently running execution context to executable code that is not associated with that execution context. The newly created execution context is pushed onto the stack and becomes the running execution context.</p>
-    <p>An execution context contains whatever implementation specific state is necessary to track the execution progress of its associated code. Each execution context has at least the state components listed in <emu-xref href="#table-state-components-for-all-execution-contexts"></emu-xref>.</p>
+    <p>An <dfn variants="execution contexts">execution context</dfn> brings together information that is relevant for the runtime evaluation of some unit of code (e.g., a script, a module, or a function object). At any point in time, there is at most one execution context per agent whose associated code is being evaluated. This is known as the agent's <dfn id="running-execution-context" variants="running execution contexts">running execution context</dfn>. All references to the running execution context in this specification denote the running execution context of the surrounding agent.</p>


Suggested change

An <dfn variants="execution contexts">execution context</dfn> brings together information that is relevant for the runtime evaluation of some unit of code (e.g., a script, a module, or a function object). At any point in time, there is at most one execution context per agent whose associated code is being evaluated. This is known as the agent's <dfn id="running-execution-context" variants="running execution contexts">running execution context</dfn>. All references to the running execution context in this specification denote the running execution context of the surrounding agent.

An <dfn variants="execution contexts">execution context</dfn> brings together information that is relevant for the runtime evaluation of some unit of code (e.g., a script, a module, or a function object). At any point in time, there is at most one execution context per agent whose associated code is being evaluated. This is known as the agent's <dfn id="running-execution-context" variants="currently running execution context">running execution context</dfn>. All references to the running execution context in this specification denote the running execution context of the surrounding agent.

it should never have to be plural since there can only be one

we say "currently" before it below, and I'd rather that just be included in an alias for this term

michaelficarra · 2024-10-24T19:33:15Z

spec.html

@@ -48832,7 +48856,7 @@ <h1>
        <emu-alg>
          1. Let _runningContext_ be the running execution context.
          1. Let _asyncContext_ be a copy of _runningContext_.
-          1. NOTE: Copying the execution state is required for AsyncBlockStart to resume its execution. It is ill-defined to resume a currently executing context.
+          1. NOTE: Copying is required because AsyncFunctionStart has different roles for _asyncContext_ and _runningContext_ that cannot (easily) be filled by a single execution context.


I think this note could actually use some expansion, but I don't have a concrete suggestion at the moment.

michaelficarra

LGTM other than nits

jmdyck added the editorial change label Nov 22, 2022

jmdyck force-pushed the transfer branch from 514b0b4 to 6e98e38 Compare November 22, 2022 22:55

jmdyck force-pushed the transfer branch from 6e98e38 to 94b2244 Compare January 14, 2023 01:46

bakkot added the editor call to be discussed in the next editor call label Jan 18, 2023

michaelficarra reviewed Jan 25, 2023

View reviewed changes

spec.html Outdated Show resolved Hide resolved

jmdyck force-pushed the transfer branch from 94b2244 to 45b4498 Compare February 13, 2023 16:33

jmdyck force-pushed the transfer branch from e0d8e8f to 9967f33 Compare March 1, 2023 01:42

michaelficarra removed the editor call to be discussed in the next editor call label Mar 1, 2023

michaelficarra mentioned this pull request Mar 7, 2023

Editorial: mark a note step as a note #3026

Merged

jmdyck force-pushed the transfer branch from 9967f33 to 3b7e84b Compare March 18, 2023 14:38

jmdyck force-pushed the transfer branch from 3b7e84b to b611e23 Compare March 22, 2023 00:46

jmdyck force-pushed the transfer branch from b611e23 to 0dbc50e Compare April 20, 2023 22:10

jmdyck force-pushed the transfer branch from 0dbc50e to e19db45 Compare August 17, 2023 23:01

michaelficarra reviewed Aug 30, 2023

View reviewed changes

jmdyck force-pushed the transfer branch from e19db45 to 6c504e9 Compare September 3, 2023 01:18

jmdyck force-pushed the transfer branch from 6893234 to 1f40ed7 Compare January 11, 2024 01:22

michaelficarra mentioned this pull request Jul 24, 2024

Normative: Propagate active ScriptOrModule with JobCallback Record #3195

Open

jmdyck force-pushed the transfer branch from 1f40ed7 to a0c2ace Compare August 10, 2024 14:49

jmdyck added 15 commits August 14, 2024 09:12

Transfer control: Replace the 6 "Resume the suspended evaluation" steps

3e61137

Transfer control: Replace the 3 remaining blocking-Resume steps

7ba5af3

(Transfer control:) Refactor GeneratorStart's Abstract Closure so tha…

ccde399

…t it has only one Return step.

Transfer control: Re-word some steps that talk about resuming executi…

53529a2

…on contexts

Transfer control: Define "transfer control"

3f51b00

Execution Context section: Move a paragraph

1867861

Take a paragraph that talks about LexicalEnvironment and VariableEnvironment, and move it to a better spot.

Eliminate certain wording around execution contexts

273e753

Specifically, eliminate wording such as: - an EC tracks the evaluation/execution of code - an EC evaluates/executes code - an EC can wait for an event - code is 'within' an EC - code is evaluated within an EC - control enters an EC

fixup: Insert "and async functions" into note re execution contexts

8d382e0

fixup: "The above step [never] completes" -> "This step is [never] re…

d5a8804

…ached"

jmdyck force-pushed the transfer branch from a0c2ace to d5a8804 Compare August 14, 2024 15:00

michaelficarra reviewed Oct 24, 2024

View reviewed changes

michaelficarra approved these changes Oct 24, 2024

View reviewed changes

		1. Remove _scriptContext_ from the execution context stack and restore the execution context that is at the top of the execution context stack as the running execution context.
		1. Assert: The execution context stack is not empty.

	1. NOTE: The above step never completes, because control is never transferred back to _acGenContext_.
	1. NOTE: This step is never reached because control is never transferred back to _acGenContext_.

	1. NOTE: The above step completes only if/when control is transferred back to _methodContext_.
	1. NOTE: This step is reached only if/when control is transferred back to _methodContext_.

	<p>To express the semantics of generators, this document uses steps of the form “<dfn id="transfer-control" variants="transfers control,transfer of control">Transfer control</dfn> from _aContext_ to _bContext_, passing _completion_”, where _aContext_ and _bContext_ are execution contexts and _completion_ is a Completion Record. This means:</p>
	<p>To express the semantics of generators, this document uses steps of the form “<dfn id="transfer-control" variants="transfer control,transfers control,transfer of control">Transfer control</dfn> from _aContext_ to _bContext_, passing _completion_”, where _aContext_ and _bContext_ are execution contexts and _completion_ is a Completion Record. This means:</p>

	<li>Execution of the current algorithm (the one with the “Transfer control” step) is suspended, and a representation of that suspended execution is saved in the code evaluation state of _aContext_.</li>
	<li>Execution of the current algorithm (the one with the “transfer control” step) is suspended, and a representation of that suspended execution is saved in the code evaluation state of _aContext_.</li>

	1. NOTE: This step is reached only if/when control is transferred back to _genContext_.
	1. NOTE: This step is reached only when control is transferred back to _genContext_, which might not happen.

Editorial: Various changes around execution contexts #2962

Are you sure you want to change the base?

Editorial: Various changes around execution contexts #2962

Conversation

jmdyck commented Nov 22, 2022

jmdyck commented Nov 22, 2022

jmdyck commented Jan 14, 2023

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

jmdyck Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

michaelficarra commented Jan 25, 2023

jmdyck commented Jan 25, 2023

jmdyck commented Feb 13, 2023

jmdyck commented Feb 13, 2023

jmdyck commented Mar 1, 2023

jmdyck commented Mar 18, 2023

jmdyck commented Mar 22, 2023

jmdyck commented Apr 20, 2023

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

jmdyck commented Jan 11, 2024

jmdyck commented Aug 10, 2024

michaelficarra commented Oct 23, 2024

jmdyck commented Oct 23, 2024

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

michaelficarra left a comment

Choose a reason for hiding this comment

jmdyck Jan 25, 2023 •

edited

Loading