Add "low" flag to launch processes as low priority. #4162

bgianfo · 2019-02-10T05:07:17Z

When building large MsBuild code bases, you often
would like to be able to use your computer while
the build is running in the background. Historically
the internal Microsoft build tools have supported
such functionality by running build processes as
low priority.

This change brings this functionality to MsBuild.

cdmihai · 2019-02-11T19:07:51Z

Looks good. I'm okay with the change, provided the following:

Support this for non windows as well. There might be an easier way to implement this crossplatform. Assuming that in all OSes the priority of the child process is inherited, you could just set the current process priority to low in XMake.cs, after parsing the cmd line arguments. As far as I know Linux processes inherit everything from the parent, and Windows child processes sometimes inherit the priority when:

... if none of the priority class flags is specified, the priority class defaults to NORMAL_PRIORITY_CLASS unless the priority class of the creating process is IDLE_PRIORITY_CLASS or BELOW_NORMAL_PRIORITY_CLASS. In this case, the child process receives the default priority class of the calling process.

Alternatively, skip crossplatform support but open an up-for-grabs issue on it

Add a test which asserts that a spawned node is in low priority. Easiest way that I can think of is to do an out of proc build (BuildParameters.DisableInProcNode = true), call a task that returns the process priority, and assert it's low priority. You can put it in BuildManager_Tests which contains other tests doing out of proc builds.

bgianfo · 2019-02-12T17:15:07Z

Thanks @cdmihai, setting it through the Process object model does seem to work, and sub processes inherit it correctly.

With this change, it doesn't seem possible to write a test how you suggested though, since argument parsing options will never be processed by the new build node (unless I'm missing something). Should I write a test like those found in XMake_Test.cs which passes the command line argument to msbuild, and a task which returns it's process priority?

cdmihai · 2019-02-12T17:57:36Z

Hmm, I see what you mean, without this being part of BuildParameters, it is impossible to react to it from inside the engine. So a question is, should this be a command line only feature, or an engine feature. I would go for command line only, as this does not seem to make sense for programatic API based access. If another process wants to use the BuildManager for low priority builds then that process can just set the priority instead. More so, it does not make sense to use multiple build managers with different priorities within the same process as they would stomp on each other, with last one winning.

Regarding tests, you are right, with this being a command line only test, you'd have to follow the pattern of the XMake_Test and set the environment variable MSBUILDNOINPROCNODE to value 1 to force out of proc builds. You can set the environment variable with a TestEnvironment, which resets all system mutations when it's disposed. And then assert against a task that prints the priority. Sadly the test is more complicated than the feature itself.

bgianfo · 2019-02-12T19:56:41Z

Test added.

One thing I realized and had to fix in my tests, is that when building with low priority and with node re-use turned on, it's possible you could land on a build node that was previously launched as a priority which does't match the expectations of the msbuild frontend.

This is kind of a weird bug and might lead to unexpected behavior.
Thoughts?

Pilchie · 2019-02-12T20:13:03Z

FYI @CyrusNajmabadi - this is similar to something you wanted from vbcscompiler, but better in a lot of ways, since it will effect other processes to.

@davkean / @jjmew - is this something we should think about exposing when VS launches builds?

rainersigwald · 2019-02-12T20:26:47Z

it's possible you could land on a build node that was previously launched as a priority which does't match the expectations of the msbuild frontend

Would it be reasonable to add process priority to the node handshake? The only downside I could see is that if there were a situation where priority wasn't inherited (for whatever reason) we might fail to connect to a just-launched node.

cdmihai · 2019-02-12T21:58:28Z

it's possible you could land on a build node that was previously launched as a priority which does't match the expectations of the msbuild frontend

Would it be reasonable to add process priority to the node handshake? The only downside I could see is that if there were a situation where priority wasn't inherited (for whatever reason) we might fail to connect to a just-launched node.

I guess you could use /nodereuse:false with /lowpriority. Since the nodes are already slow, won't matter if some minor process startup and jitting time is spent as well :)

rainersigwald · 2019-02-12T22:01:58Z

To take that a step further, /low could imply /nr:false.

davkean · 2019-02-12T23:51:35Z

I don't think /low implies /nr:false. Low means to me, do the build as fast as possible just don't interrupt my other applications that I'm using.

Does this switch affect the vbcscompiler launch? If so, what if it's already open?

cdmihai · 2019-02-13T00:24:58Z

Does this switch affect the vbcscompiler launch? If so, what if it's already open?

If we'd want to reach out to task initiated build servers then we'd have to go down the route where the low priority gets stored in BuildParameters, and then we expose it to tasks via IBuildEngine. That way tasks, if they want to, can propagate the information to their servers, which, if they want to, can lower their priority for the duration of the request.

Though honestly for now i'd just go with this simple implementation and we can beef it up later based on feedback.

bgianfo · 2019-02-27T17:02:15Z

It looks like we never reached consensus on the node reuse problem. Should I pursue making the low priority flag part of the node handshake given @davkean's objection to forcing /nr;flase?

src/MSBuild/XMake.cs

src/MSBuild.UnitTests/XMake_Tests.cs

src/MSBuild/Resources/xlf/Strings.pl.xlf

rainersigwald · 2019-07-03T20:05:58Z

Talked this over with @benvillalobos and @livarcocc. We think adding priority to the node handshake sounds like a good path forward. @bgianfo, would you mind doing that? #4337 recently touched some of the handshake calculation and would be a good place to look. Let me know if you have any problems (or don't have time to do this now, we can pick it up from here if you're swamped).

bgianfo · 2019-07-03T22:34:52Z

@rainersigwald I'll take a crack at it, will report back if I don't think I'll be able to get it done for whatever reason.

bgianfo · 2019-07-08T18:48:39Z

@rainersigwald is there a known issue with the tests hitting 1 hour timeout in Azure Devops?

rainersigwald · 2019-07-08T18:49:50Z

No, but I've seen it a lot today, please assume you're innocent until further notice.

bgianfo · 2019-07-10T06:38:41Z

Alright, I believe I have the node handshake coded up correctly.. all tests are passing.

bgianfo · 2019-07-15T18:09:00Z

Any chance of this getting reviewed this week?

cdmihai · 2019-07-15T18:12:56Z

@bgianfo
We're going to discuss it in tomorrow's PR meeting, and decide on whether it needs more work, or it's good to go.

rainersigwald

Made some comments.

Team triage: do it the easy way and just call ShutdownAllNodes with both priorities. If that causes a noticeable perf problem we'll deal with it then.

Should be good to merge after that. Thanks for your patience!

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs

src/Build/BackEnd/BuildManager/BuildParameters.cs

When building large MsBuild code bases, you often would like to be able to use your computer while the build is running in the background. Historically the internal Microsoft build tools have supported such functionality by running build processes as low priority. This change brings this functionality to MsBuild.

bgianfo · 2019-12-18T01:07:03Z

Last time I looked at this and rebased the PR, the tests are now failing in what appear to be consistently failing with some bug where the invoked MsBuild isn't outputting the build priority. The output looks like an empty build project... Spent a few hours debugging what's going on and didn't get very far. ae5c2d8 is one example of me trying to get more data out of CI.

@rainersigwald Any debugging idea's?

Forgind · 2020-01-03T19:10:27Z

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs

+            // down all the nodes on exit, we will attempt to shutdown
+            // all matching notes with and without the priroity bit set.
+            // So precompute both versions of the handshake now.
+            long hostHandshake = NodeProviderOutOfProc.GetHostHandshake(nodeReuse, enableLowPriority: false);


nit:
I'd prefer if these two were inlined.

Forgind · 2020-01-03T19:18:06Z

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs

@@ -121,7 +122,17 @@ protected void ShutdownAllNodes(long hostHandshake, long clientHandshake, NodeCo
            // For all processes in the list, send signal to terminate if able to connect
            foreach (Process nodeProcess in nodeProcesses)
            {
-                Stream nodeStream = TryConnectToProcess(nodeProcess.Id, 30/*verified to miss nodes if smaller*/, hostHandshake, clientHandshake);
+                // Verified to miss nodes if smaller.
+                int timeout = 30;


It might be better to start nodes with the same priority as normal but quickly switch to low priority afterwards.

Nodes started with low priority are more likely to be killed between when they're started and when they finish the handshake. This leads to the hang in this issue

If there's a 30-second timeout, and the node gets swapped out before then, this will fail.

Node creation is expected to be fast (<< 30 seconds), so a brief moment of high priority will likely be unnoticed.

I started a change to make node creation high priority, but I haven't finished it yet. It might be reasonable to combine the two if you like the idea of starting at high priority.

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs

Forgind · 2020-01-03T19:31:46Z

src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs

+            bool unquoteParameters;
+            bool emptyParametersAllowed;
+
+            Assert.True(CommandLineSwitches.IsParameterizedSwitch("low", out parameterizedSwitch, out duplicateSwitchErrorMessage, out multipleParametersAllowed, out missingParametersErrorMessage, out unquoteParameters, out emptyParametersAllowed));


Switch to Shouldly for all these.

Forgind · 2020-01-03T19:35:12Z

src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs

+            Assert.True(unquoteParameters);
+            Assert.False(emptyParametersAllowed);
+
+            Assert.True(CommandLineSwitches.IsParameterizedSwitch("LOW", out parameterizedSwitch, out duplicateSwitchErrorMessage, out multipleParametersAllowed, out missingParametersErrorMessage, out unquoteParameters, out emptyParametersAllowed));


It would probably be better to switch these to a theory instead of copying ~the same code 4 times. Then you could also use CommandLineSwitches.IsParameterizedSwitch("low", out CommandLineSwitches.ParameterizedSwitch parameterizedSwitch and the same for the others to save a few more lines.

Forgind · 2020-01-03T19:35:29Z

src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs

+            Assert.True(unquoteParameters);
+            Assert.False(emptyParametersAllowed);
+
+            Assert.True(CommandLineSwitches.IsParameterizedSwitch("lowpriority", out parameterizedSwitch, out duplicateSwitchErrorMessage, out multipleParametersAllowed, out missingParametersErrorMessage, out unquoteParameters, out emptyParametersAllowed));


nit:
Please split lines this long across multiple lines.

Forgind · 2020-01-03T19:40:33Z

src/MSBuild/Resources/Strings.resx

@@ -787,6 +787,20 @@ Copyright (C) Microsoft Corporation. All rights reserved.
      LOCALIZATION: None of the lines should be longer than a standard width console window, eg 80 chars.
    </comment>
  </data>
+  <data name="HelpMessage_37_LowPrioritySwitch" UESanitized="false" Visibility="Public">


UESanitized hasn't been used since 2004.

cdmihai · 2020-02-04T18:03:46Z

src/MSBuild/XMake.cs

+                    // priority and let sub processes inherit that priority.
+                    if (lowPriority)
+                    {
+                        Process currentProc = Process.GetCurrentProcess();


I'd move all these priority changes to a new utility class, with commands like PriorityUtils.SwitchToUserLowPriority that encapsulate the meaning of a "user wants to do other things with their computer" request and how to achieve it. Another use future potential use case for the util class would be encapsulating the meaning of "node spawned and needs high priority to respond back". These commands could also return an IDisposable for automatic state undoing, like using(var handshakePriority = PriorityUtils.SwitchToNodeHandshakePriority){// handshake}

cdmihai · 2020-02-04T18:15:47Z

src/MSBuild/XMake.cs

@@ -1114,6 +1126,7 @@ string outputResultsCache
                    }

                    parameters.EnableNodeReuse = enableNodeReuse;
+                    parameters.LowPriority = lowPriority;


I'd rename this to something else than BuildParameters.LowPriority, because it looks like there might be multiple intents to playing around with priority, and the actual process/thread priority for one intent might be different than the other. How about making it a flag enum instead, something like BuildParameters.PriorityMode, where one value might be UserLowPriority, another future one might be NodeHandshake. The good part is that we can keep adding new values without changing the public API. Then the new PriorityUtils class would take in this enum and know what to do based on its values.

Is there any reason not to reuse the ProcessPriorityClass enum?

Also, since there is no longer another intention to change priority in the immediate future, I'm thinking it's better to leave it as LowPriority. It's messier if we want to go in and change it again, but I started making it fully generic and ran into difficulties with ShutdownAllNodes, since if it can take any value in PriorityClass, I'd either need a hash function that allocates specific bits (or equivalent) for the priority to make it easy to generate all relevant priorities or accept that I'll miss some nodes or shut down too many. Seems like too much headache for more generality that we might not use. I'll still make the PriorityUtils.SwitchProcessPriorityTo take a ProcessPriorityClass so it can be reasonably generic.

cdmihai · 2020-02-04T20:58:50Z

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs

@@ -121,7 +122,17 @@ protected void ShutdownAllNodes(long hostHandshake, long clientHandshake, NodeCo
            // For all processes in the list, send signal to terminate if able to connect
            foreach (Process nodeProcess in nodeProcesses)
            {
-                Stream nodeStream = TryConnectToProcess(nodeProcess.Id, 30/*verified to miss nodes if smaller*/, hostHandshake, clientHandshake);
+                // Verified to miss nodes if smaller.


Ambiguous comment.

danmoseley · 2020-03-06T22:36:41Z

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs

        {
            long baseHandshake = Constants.AssemblyTimestamp;

            baseHandshake = baseHandshake*17 + EnvironmentUtilities.Is64BitProcess.GetHashCode();

            baseHandshake = baseHandshake*17 + enableNodeReuse.GetHashCode();

+            baseHandshake = baseHandshake*17 + enableLowPriority.GetHashCode();


BTW if you do need more bits (eg., to specify the fine grained priority) you can probably remove the masking of the first byte that is done in GetClientHandshake(). This masking was needed when nodes might exist running versions of MSBuild written before ~2008 (?) ... which I think is now unlikely.

As you pointed out below, there aren't specific bits for features anymore, but I could theoretically take advantage of this masking to make the first byte the priority. (I could really probably use the first four bits.) That would be a bit complicated, though, so I'm not sure it's worth it.

Both sides of the handshake should be the same exact binaries, so this could theoretically be changed to be bit flipping. Not suggesting you do that, but it's not as if this scheme is locked in.

(Well - except that if this scheme is making a truly universally distributed bit pattern - then it actually is impossible to be certain that you won't accidentally connect to it. Unless you make the handshake longer. But now we're being even more implausible.)

danmoseley · 2020-03-06T22:39:10Z

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs

        /// </summary>
        /// <param name="enableNodeReuse">Is reuse of build nodes allowed?</param>
-        internal static long GetHostHandshake(bool enableNodeReuse)
+        /// <param name="enableLowPriority">Is the build running at low priority?</param>
+        internal static long GetHostHandshake(bool enableNodeReuse, bool enableLowPriority)
        {
            long baseHandshake = Constants.AssemblyTimestamp;

            baseHandshake = baseHandshake*17 + EnvironmentUtilities.Is64BitProcess.GetHashCode();


Aside because it's not changed by your PR, but I hadn't noticed that the hashing scheme was now mixing all the bits. There is of course the possibility, however small, that that could cause a match when there should not be. Consider eg., if we changed the impolementation of string.GetHashCode() to return all zeros - that's absurd but valid. I thought originally this method was setting specific bits for specific features. Eg., 64 bitness could toggle one bit. Not sure why it's mixing the bits now.

cdmihai's

rainersigwald · 2020-03-13T17:07:35Z

src/Build/BackEnd/BuildManager/BuildParameters.cs

+        /// <summary>
+        /// Gets or sets a value indicating whether the build process should run as low priority.
+        /// </summary>
+        public bool LowPriority { get; set; }


I'm surprised to see this implemented as an autoproperty instead of with an explicit backing field. Does it really not need to survive serialization? If not, please add an explicit comment in the translator like this one

https://github.com/microsoft/msbuild/blob/9ebbc579bd48e9a0da4ec52b8605e89fca73af9c/src/Build/BackEnd/BuildManager/BuildParameters.cs#L840

Would you mind providing more details on when something needs to survive serialization?

Priority passes automatically from parent process to child process, and it's also passed explicitly as a parameter at node creation. (Technically, we could get it from the process and avoid passing that around, but it would've made it harder if we had taken the high priority node creation change.) Is that sufficient to say it doesn't need to be translated or are there other necessary conditions?

rainersigwald · 2020-03-13T17:12:28Z

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs

-            ShutdownAllNodes(NodeProviderOutOfProc.GetHostHandshake(nodeReuse), NodeProviderOutOfProc.GetClientHandshake(), NodeContextTerminated);
+            // To avoid issues with mismatched priorities not shutting
+            // down all the nodes on exit, we will attempt to shutdown
+            // all matching notes with and without the priority bit set.


Suggested change

// all matching notes with and without the priority bit set.

// all matching nodes with and without the priority bit set.

rainersigwald · 2020-03-16T11:35:15Z

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs

@@ -121,7 +122,17 @@ protected void ShutdownAllNodes(long hostHandshake, long clientHandshake, NodeCo
            // For all processes in the list, send signal to terminate if able to connect
            foreach (Process nodeProcess in nodeProcesses)
            {
-                Stream nodeStream = TryConnectToProcess(nodeProcess.Id, 30/*verified to miss nodes if smaller*/, hostHandshake, clientHandshake);
+                // Some nodes can take up to this long to respond, so this is the smallest it can be without erroneously missing nodes.


This is what the original comment meant, but let’s add context to the magic number. I believe this was a 2013 comment that didn’t provide methodology. Best to say that.

I imagine they just created a lot of processes, then ran a build to see if it would pick up the extant nodes, but I wasn't there in 2013. What was the methodology?

You don't have to describe the methodology, just don't state it as fact now. Otherwise, future me might email future you in five years demanding an explanation of how you knew this. And you won't know!

The internal commit that introduced this comment was http://vstfdevdiv:8080/DevDiv2/DevDiv/_versionControl/changeset/865139?_a=compare&path=%24%2FDevDiv%2Ffeature%2FVSPro_VBCS%2Fvsproject%2Fxmake%2FXMakeBuildEngine%2FBackEnd%2FComponents%2FCommunications%2FNodeProviderOutOfProcBase.cs

rainersigwald · 2020-03-16T11:39:06Z

src/MSBuild/MSBuild.csproj

-          DependsOnTargets="ResolveAssemblyReferences"
-          BeforeTargets="AssignTargetPaths"
-          Condition="'$(MonoBuild)' != 'true'">
+  <Target Name="AddRefAssemblies" DependsOnTargets="ResolveAssemblyReferences" BeforeTargets="AssignTargetPaths" Condition="'$(MonoBuild)' != 'true'">


Can this reformatting be reverted?

rainersigwald · 2020-03-16T11:40:29Z

src/MSBuild/PriorityUtils.cs

+{
+    internal class PriorityUtils
+    {
+        public static void SwitchProcessPriorityTo(ProcessPriorityClass priority)


This seems a bit much. Is the catch necessary now?

It is if we want to avoid crashing with no helpful message on certain operating systems if a user wants to use this to switch a node to a higher priority, even if that higher priority is changing from low priority to normal priority (which sounds very reasonable). That makes it worth it to me.

How would a user switch a node to a higher priority? The only flag we're exposing is low, right?

bgianfo · 2020-03-18T22:01:34Z

Thanks for bringing this over the finish line @Forgind!

danmoseley reviewed Mar 20, 2019

View reviewed changes

src/MSBuild/XMake.cs Outdated Show resolved Hide resolved

danmoseley reviewed Mar 20, 2019

View reviewed changes

src/MSBuild.UnitTests/XMake_Tests.cs Show resolved Hide resolved

danmoseley reviewed Mar 20, 2019

View reviewed changes

src/MSBuild/Resources/xlf/Strings.pl.xlf Show resolved Hide resolved

rainersigwald reviewed Jul 16, 2019

View reviewed changes

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs Show resolved Hide resolved

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProc.cs Outdated Show resolved Hide resolved

rainersigwald added this to the MSBuild 16.4 milestone Aug 20, 2019

wjk suggested changes Sep 22, 2019

View reviewed changes

src/Build/BackEnd/BuildManager/BuildParameters.cs Outdated Show resolved Hide resolved

src/Build/BackEnd/BuildManager/BuildParameters.cs Outdated Show resolved Hide resolved

livarcocc assigned rainersigwald Oct 8, 2019

rainersigwald modified the milestones: MSBuild 16.4, MSBuild 16.5 Oct 15, 2019

rainersigwald modified the milestones: MSBuild 16.5, MSBuild 16.5 Preview 1 (Sprint 161) Oct 25, 2019

livarcocc modified the milestones: 16.5 Preview3, MSBuild 16.5 Preview 3 Dec 11, 2019

Forgind requested changes Jan 3, 2020

View reviewed changes

rainersigwald modified the milestones: MSBuild 16.5 Preview 3, MSBuild 16.6 Preview1 Jan 24, 2020

Fix tests and other errata

03f746d

Forgind approved these changes Jan 29, 2020

View reviewed changes

Forgind requested a review from cdmihai February 3, 2020 23:33

cdmihai reviewed Feb 4, 2020

View reviewed changes

rainersigwald modified the milestones: MSBuild 16.6 Preview1, MSBuild 16.6 Preview 2 Feb 24, 2020

danmoseley reviewed Mar 6, 2020

View reviewed changes

Forgind added 5 commits March 6, 2020 14:48

Address comments

cbf77f5

cdmihai's

Merge branch 'master' into low-priority-flag

c705370

Fix build issues

8e83952

Fix nonexistent reference

ede4cfe

Do not revert priority

9ebbc57

rainersigwald reviewed Mar 16, 2020

View reviewed changes

Forgind added 2 commits March 17, 2020 17:27

Comments

9378000

Removed reference to deleted file

7869acf

Forgind merged commit b111470 into dotnet:master Mar 18, 2020

bgianfo deleted the low-priority-flag branch March 18, 2020 22:07

dagood mentioned this pull request May 21, 2020

Update 3.4.1 breaks building Apache Thrift (permission issue?) on Ubuntu 18.04 dotnet/core#4719

Closed

KirillOsenkov mentioned this pull request May 27, 2020

64-bit node processes load MSBuild binaries from 32-bit location #5375

Closed

rainersigwald mentioned this pull request May 27, 2020

Unconditionally attempting to set process priority fails in some environments #5365

Closed

	// all matching notes with and without the priority bit set.
	// all matching nodes with and without the priority bit set.

Add "low" flag to launch processes as low priority. #4162

Add "low" flag to launch processes as low priority. #4162

Conversation

bgianfo commented Feb 10, 2019

cdmihai commented Feb 11, 2019

bgianfo commented Feb 12, 2019

cdmihai commented Feb 12, 2019 • edited

bgianfo commented Feb 12, 2019 • edited

Pilchie commented Feb 12, 2019 • edited

rainersigwald commented Feb 12, 2019

cdmihai commented Feb 12, 2019 • edited

rainersigwald commented Feb 12, 2019

davkean commented Feb 12, 2019

cdmihai commented Feb 13, 2019

bgianfo commented Feb 27, 2019

rainersigwald commented Jul 3, 2019

bgianfo commented Jul 3, 2019

bgianfo commented Jul 8, 2019

rainersigwald commented Jul 8, 2019

bgianfo commented Jul 10, 2019

bgianfo commented Jul 15, 2019

cdmihai commented Jul 15, 2019

rainersigwald left a comment

Choose a reason for hiding this comment

bgianfo commented Dec 18, 2019

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

cdmihai Feb 4, 2020 • edited

Choose a reason for hiding this comment

cdmihai Feb 4, 2020 • edited

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

bgianfo commented Mar 18, 2020

cdmihai commented Feb 12, 2019 •

edited

bgianfo commented Feb 12, 2019 •

edited

Pilchie commented Feb 12, 2019 •

edited

cdmihai commented Feb 12, 2019 •

edited

cdmihai Feb 4, 2020 •

edited

cdmihai Feb 4, 2020 •

edited