Native pipe #2450

vors · 2016-10-11T02:06:00Z

Fix #559
Improve pipeline for native commands

Start native process in Prepare() instead of Complete()
in NativeCommandProcessor.
Remove unneeded input, output, error threads. Replaced by
simpler primitives.

Motivation example:

ping | %{powershell-code} | grep now works in real-time.
See the test

TODO before merging:

replace spin-lock by something like notify/wait
resurrect and test minishell with custom host serialization
Changelog entry
Fix feature tests for

PowerShell/test/powershell/Host/ConsoleHost.Tests.ps1

Line 57 in ae7b857

$a = 1,2,3
Investigate, is this test flaky or not https://ci.appveyor.com/project/PowerShell/powershell/build/6.0.0-alpha.12-1287/tests

msftclas · 2016-10-11T02:06:04Z

Hi @vors, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Sergei Vorobev). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

daxian-dbw · 2016-10-19T02:41:50Z

@vors could you please resolve the Travis CI failures?

vors · 2016-10-19T03:02:19Z

@daxian-dbw working on it.
There are two parts: PSDSC.psm1 need to be updated: it uses sort instead of sort-object in one place and worked by accident #2520. The second part is about handing native executable failures because it still should not produce broken pipe error.

vors · 2016-10-22T03:33:15Z

🎉 @daxian-dbw ready for review

daxian-dbw · 2016-10-26T03:56:58Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

@@ -140,6 +142,8 @@ internal ProcessOutputObject(object data, MinishellStream stream)
    /// </summary>
    internal class NativeCommandProcessor : CommandProcessorBase
    {
+        private static string XmlCliTag = "#< CLIXML";


It's better to make it a const variable.

daxian-dbw · 2016-10-26T19:25:05Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+                _exceptionToRethrow = e;
+
+            } // try
+            catch (PipelineStoppedException)


maybe we shouldn't catch this exception since it's just re-thrown.

By design #Closed

daxian-dbw · 2016-10-26T20:46:51Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+                        {
+                            bool isFirstOutput = true;
+                            bool isXmlCliOutput = false;
+                            _nativeProcess.OutputDataReceived += new DataReceivedEventHandler((sender, e) =>


By using DataReceivedEventHandler, you won't be able to pipe byte streams between native commands, as e.Data is already a string encoded with the default encoding.
In order to pipe streams between native commands, we need to have a separate thread reading from the redirected stream of the upstream command and writing to the downstream command -- something similar to the current ProcessOutputReader, but with the ability to read/write bytes directly.

Here is my 2 cents: keep the ProcessOutputReader and ProcessStreamReader, but re-design them to make it also able to transmit byte stream, and also get rid of the PipelineWriter (~~if a concurrent queue is sufficient enough~~ I think BlockingCollection<T> is a better choice).

What you are describing is #1908 and it's outside of the scope of this PR. I'd like to solve it with creating already piped processes and avoid expensive manual marshaling from stdout of one to stdin of another.

Yes, I know #1908 is not covered by this PR. My point is that we would still need a thread worker to transfer byte streams between native commands, so probably we should keep the ProcessOutputReader implementation there for now just in case that we need to reuse some of that code.
And for the same reason, using OutputDataReceived and ErrorDataReceived to collect process output/error may not be appropriate since we probably have to change it again using a thread worker when addressing #1908

@vors found that Stream.CopyToAsync is a perfect solution for this scenario - the async mechanism in CLR saves use the effort to own the threading implementation. Given that, the ProcessOutputReader will no longer be needed. #Closed

daxian-dbw · 2016-10-26T21:39:14Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+        /// </returns>
+        private bool ConsumeAvailableNativeProcessOutput()
+        {
+            bool isNewData = false;


Maybe it should be hasNewData?

I removed this var

daxian-dbw · 2016-10-26T21:39:47Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+        /// <returns>
+        /// True if there was any new input available, otherwise false.
+        /// </returns>
+        private bool ConsumeAvailableNativeProcessOutput()


What is the point of returning bool from this method?

It's a left-over from previous experiments.

daxian-dbw · 2016-10-26T22:42:14Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+            if (this.Command.Context.CurrentPipelineStopping)
+            {
+                this.StopProcessing();
+                return;


the return doesn't seem necessary.

good catch. I moved it to another place.

daxian-dbw · 2016-10-26T23:02:17Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

@@ -1104,7 +1364,7 @@ private bool IsDownstreamOutDefault(Pipe downstreamPipe)
        /// <param name="redirectInput"></param>
        private void CalculateIORedirection(out bool redirectOutput, out bool redirectError, out bool redirectInput)
        {
-            redirectInput = true;
+            redirectInput = this.Command.MyInvocation.PipelineLength > 0;


The native command may be the first command in the pipeline, and it doesn't need input in that case.

Good catch, it should be 'PipelinePosition'

daxian-dbw · 2016-10-26T23:28:13Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

@@ -1352,31 +1609,46 @@ internal ProcessInputWriter(InternalCommand command)
        {
            Dbg.Assert(command != null, "Caller should validate the parameter");
            _command = command;
+
+            _pipeline = ScriptBlock.Create("Out-String -Stream").GetSteppablePipeline();


Will -Stream be a breaking change?

why? Out-String returns a single string, while Out-String -Stream returns an string array.

Hmmm. Ok, so I would need to double check this. Here is my original thinking:

Before we had all data upfront, so we can use just Out-String. Now we get new data sporadically and should still output something, even when data is incomplete. -Stream seemed like a right switch.

After your comments, I revisited this thinking and it could be indeed incorrect. I think data would still be available, there would be no aggregation. For example, lets take a look at

$h = @{1 = 2}; $h; sleep 1; $h | Out-String | %{$_}

When I run it, I get the first $h immediately.

Also -Stream adds overhead, because there are more objects. So I think you are correct. Thank you for spotting the problem!

UPD: wait, I just realised my example was wrong :)

Here is the correct one:

$h = @{1 = 2}; 1..2 | % {$h; sleep 1} | Out-String

Out-String waits for all output before outputing anything.
Where

$h = @{1 = 2}; 1..2 | % {$h; sleep 1} | Out-String -Stream

Output everything as soon as available.
For the line-ends, I would not worry. I will double check that behaviour is the same on win and unix.

Sounds good. #Closed

daxian-dbw · 2016-10-26T23:38:59Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

            {
-                return _inputList.Count;
+                string line = PSObject.ToStringParser(_command.Context, item);


It looks you are not differentiating Text input from Xml input, but you are differentiating them when processing the output of native command. Is this a oversight?

Possible, but I found that serizliation is handled by the code in src\Microsoft.PowerShell.ConsoleHost\host\msh\Serialization.cs. I think serialization path here was not used at all. I'd love to see proven wrong, if you can find a case, where it was used.

I don't know whether it's used or not. But I would like to see some analysis data if you believe it's an affordable breaking change.

XmlInput has been brought back in the new iteration.
#Closed

daxian-dbw · 2016-10-26T23:43:59Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

@@ -1352,31 +1609,46 @@ internal ProcessInputWriter(InternalCommand command)
        {
            Dbg.Assert(command != null, "Caller should validate the parameter");
            _command = command;
+
+            _pipeline = ScriptBlock.Create("Out-String -Stream").GetSteppablePipeline();
+            _pipeline.Begin(true);
        }


With your change to ProcessInputWriter, we won't be able to use the same InputWriter type to handle byte stream transfers between native commands -- we will need a thread to read byte output from upstream native command and write to the downstream command.
My opinion is to keep the "use-separate-thread" module of ProcessInputWriter, but again use a BlockingCollection between Add and the thread that does the actual write operation. It's essentially a consumer/provider problem.

The existing approach was spinning a thread and managing its lifecycle. It's pretty wasteful and unnecessary complicated. One of the my goal is to avoid a "medium" between two native processes, while implementing #1908. It would be a simple redirection without marshaling. So this would be a completely different code path. If we will try to reuse existing approach with ProcessInputWriter and enhance it for bytes, powershell would never be able to get closer to bash in native | native case by it's performance. We need to strip away the overhead of marshaling objects and just do the stream redirection.

With Stream.CopyToAsync to relay outputs between processes, we won't need our own threading implementation. #Closed

vors · 2016-10-27T02:07:23Z

I will update PR with the notify/wait code

daxian-dbw · 2016-10-28T22:47:16Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

-            }
-            _inputThread = new Thread(new ThreadStart(this.WriterThreadProc));
-            _inputThread.Start();
+            _inputFormat = inputFormat;


_inputFormat was used for processing Text or Xml input. If it's proven that we can ignore Xml input, then we need to clean up ProcessInputWriter to remove this member, including the parameter of Start method.

I found the case, when we use this code:

1,2,3 | powershell -noprofile -command { $input }

Resurrecting it now

daxian-dbw · 2016-10-28T23:13:32Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+            // we allow call Done() multiply times.
+            // For example one time from Process() code path, 
+            // when we detect that process already finished
+            // and once from End() code path.


Process() and End() is kinda confusing at first glance. Can you please change Process() to ProcessRecord() and End() to Complete()?
And please add one more comment explains that "even though Done could be called multiple times, the calls are on the same thread, so there is no race condition here".

daxian-dbw · 2016-10-28T23:17:23Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

-                    WriteXmlInput();
+                    // on unix, if process is already finished attempt to dispose it will
+                    // lead to "Broken pipe" exception.
+                    // we are ignoring it here


Minor: may be we can avoid this exception by using the overload StreamWriter(Stream stream, Encoding encoding, int bufferSize, bool leaveOpen);. The writer will not try to dispose the stream when leaveOpen = true. Let the process object handle closing the stream.

I dig into that and as far as I see, that will hang the process: there is no way to tell it that all input was present and close the stream, if process expects input.

Good point. #Closed

daxian-dbw · 2016-10-28T23:22:37Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+                                    if (isFirstOutput)
+                                    {
+                                        isFirstOutput = false;
+                                        if (e.Data == XmlCliTag)


It's recommended to a culture overload Equals method to compare strings.

daxian-dbw · 2016-10-28T23:22:47Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

                                {
-                                    lineContents.Append(bufferContents[row, column].Character);
+                                    if (e.Data == XmlCliTag)


daxian-dbw · 2016-10-28T23:49:40Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

-        {
-            get
+            Array formattedObjects = _pipeline.Process(input);
+            foreach (var item in formattedObjects)
            {


Shall we add if (_stopping) { return } in this loop?

We probably should not. That would increase code complexity (and time) for common path and save some time for uncommon path. Once all objects are formatted (in most cases there is only 1), it's pretty fast to write them to the process. We should not optimize uncommon paths, if it hurst common path perf.

daxian-dbw · 2016-10-28T23:54:35Z

test/powershell/Language/Scripting/NativeExecution/NativeMinishell.Tests.ps1

@@ -0,0 +1,50 @@
+# Minishell is a powershell concept.
+# It's primare use-case is when somebody executes a scriptblock in the new powershell process.


You mean Its primary?

You missed the typo primare.

daxian-dbw · 2016-10-31T22:22:46Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+                        {
+                            bool isFirstOutput = true;
+                            bool isXmlCliOutput = false;
+                            _nativeProcess.OutputDataReceived += new DataReceivedEventHandler((sender, e) =>


Can we encapsulate the event handlers and related variables/methods/objects into a class? I'm thinking about something like the following:

... var handler = new ProcessOutputHandler(true, true); _nativeProcess.OutputDataReceived += new DataReceivedEventHandler(handler.OutputHandler); _nativeProcess.ErrorDataReceived += new DataReceivedEventHandler(handler.ErrorHandler); ... public class ProcessOutputHandler { public ProcessOutputHandler (bool redirectOutput, bool redirectError) { Debug.Assert(redirectOutput || redirectError, "Caller should redirect at least one stream"); _blockingQueue = new BlockingCollection<Tuple<string, int>>(new ConcurrentQueue<Tuple<string, int>>()); if (redirectOutput) { _refCount ++; } if (redirectError) { _refCount ++; } } private int _refCount = 0; private bool _isFirstOutput = true, _isFirstError = true; private bool _isOutputXml = false, _isErrorXml = false; private BlockingCollection<Tuple<string, int>> _blockingQueue; public BlockingCollection<Tuple<string, int>> BlockingQueue { get { return _blockingQueue; } } private List<ProcessOutputObject> DeserializeCliXmlObject(string xml, bool isOutput) { // ... } public void OutputHandler(object sendingProcess, DataReceivedEventArgs outLine) { if (outLine.Data == null) { // StandardOutput is closed if (Interlocked.Decrement(ref _refCount) == 0) { _blockingQueue.CompleteAdding(); } } else { // Process output and add to _blockingQueue ... } } public void ErrorHandler(object sendingProcess, DataReceivedEventArgs errorLine) { if (errorLine.Data == null) { // ErrorOutput is closed if (Interlocked.Decrement(ref _refCount) == 0) { _blockingQueue.CompleteAdding(); } } else { // Process error and add to _blockingQueue ... } } }

Good idea, done

vors · 2016-11-08T02:40:25Z

@daxian-dbw I removed spin-lock and employed BlockedCollection as you suggested.

daxian-dbw · 2016-11-09T22:31:28Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+            }
+        }
+
+        private class ProcessOutputHandler


Can you please move this class outside of NativeCommandProcessor? My arguments:

Nested class usually worsens readability of the code.

It's essentially the counterpart of ProcessInputWriter, and thus I think it should be a non-nested internal class similar as ProcessInputWriter

daxian-dbw · 2016-11-09T22:43:24Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+                }
+            }
+
+            private void OutputHandler(object sender, DataReceivedEventArgs e)


Minor comment: I always encourage people to use descriptive/meaningful variable names. For example, the parameter e here and in ErrorHandler below could be a bit more descriptive, like outputReceived and errorReceived.

daxian-dbw · 2016-11-09T22:44:52Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+            private bool _isFirstError;
+            private bool _isXmlCliOutput;
+            private bool _isXmlCliError;
+            private string _path;


Minor comment: again about variable name. _processFileName would be better I think 😄

daxian-dbw · 2016-11-09T23:19:02Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

-                        //Wait for input writer to finish.
-                        _inputWriter.Done();
+                // collection is empty or exception been raised
+                return null;


This if (blocking) block is basically the same as following:

if (blocking) { // if adding was completed and the queue is empty, there is no need to do a blocking Take(), if (!_nativeProcessOutputQueue.IsCompleted) { try { return _nativeProcessOutputQueue.Take(); } catch (InvalidOperationException) { // It's a normal situation: another thread can mark collection as CompleteAdding // in a concurrent way and we will rise an exception in Take(). // Although it's a normal situation it's not the most common path // and will be executed only on the race condtion case. } } // collection is empty and adding completed, or exception been raised return null; }

Oh, cool. I was lazy to read about IsCompleted :)

Had a conversation with @lzybkr about perf here.
Leaving a common path outside of try {} will have a slight positive upside. I think it's worth to leave it as is, I will add a comment with explanation.

Bear in mind that this has nothing to do with try/catch blocks: you only incur the cost when the actual exception is thrown. You can use as many try/catch blocks as you want. Using exceptions gratuitously is where you lose performance.

This is from a MSDN article Performance Tips and Tricks in .NET Applications. It indicates that try/catch block itself doesn't incur any performance overhead, as long as no exception is thrown.

daxian-dbw · 2016-11-09T23:25:27Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+                    //Wait for the process to exit and consume available output
+                    while (!_nativeProcess.HasExited)
+                    {
+                        ConsumeAvailableNativeProcessOutput(blocking: true);


This shouldn't be within while (!_nativeProcess.HasExited). It's possible that the process quickly writes a lot output and exits, but the output has been captured in our _nativeProcessOutputQueue, and we can still read all those output and process them even if the process is gone. Otherwise, we might miss some some output/error because we stop reading the queue when the process is gone.

Good catch! It's a left-over from the previous spin-lock approach.

daxian-dbw · 2016-11-09T23:26:29Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

-                            StringBuilder bufferText = new StringBuilder();
+                    // read all the available output one more time
+                    _nativeProcess.WaitForExit();
+                    ConsumeAvailableNativeProcessOutput(blocking: true);


This ConsumeAvailableNativeProcessOutput(blocking: true); won't be necessary. One blocking reading of the queue is guaranteed to read all output/error out of it.

- Move ProcessOutputHandler outside of NativeCommmandProcessor - Remove redundent ConsumeAvailableNativeProcessOutput() blocking call

Also move it to the Feature category

vors · 2016-11-16T00:40:23Z

Ready for review!

daxian-dbw · 2016-11-16T01:07:50Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

-                // nothing to do
-            }
-            else if (line.Equals("#< CLIXML", StringComparison.Ordinal) == false)
+            // we allow call Done() multiply times.


Done() should be Dispose() here.

daxian-dbw · 2016-11-16T01:09:29Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

-            _processOutput = new ObjectStream(128);
+            if (_stopping || _streamWriter == null)
+            {
+                // if _streamWriter is already null, then we already called Done()


Done() should be Dispose() here.

daxian-dbw · 2016-11-16T01:10:57Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

-                    }
-                    AddObjectToWriter(obj, stream);
+                    _xmlSerializer.Done();
+                    Dispose();


This Dispose() is unnecessary as it's called in the end of this method.

oh, good catch! I didn't remove it from here

daxian-dbw · 2016-11-16T01:11:10Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+                // if the downstream process already exited.
+                // to Prevent it, we first finalize the pipeline and set it to null,
+                // then calling the result processing for the last time
+                AddTextInputFromFormattedArray(finalResults);


daxian-dbw · 2016-11-16T01:15:21Z

I don't see your updates on the duplicate _nativeProcessOutputQueue.Take(). I still think it's unnecessary duplication of code. #Closed

…essor.cs

daxian-dbw · 2016-11-16T01:40:42Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+        {
+            if (blocking)
+            {
+                if (!_nativeProcessOutputQueue.IsCompleted)


Can we please keep the old comment // if adding was completed and the queue is empty, there is no need to do a blocking Take()? It will help reader to understand why we check IsCompleted which is a great optimization to avoid unnecessary exception from Take().

daxian-dbw

LGTM. Thank you for getting this work done!

iSazonov · 2016-11-16T04:39:39Z

Great! 🌟

msftclas added the cla-not-required label Oct 11, 2016

vors force-pushed the native-pipe branch 2 times, most recently from 38da61b to c4b357a Compare October 12, 2016 23:34

daxian-dbw added Review - Needed labels Oct 19, 2016

daxian-dbw self-assigned this Oct 19, 2016

vors force-pushed the native-pipe branch 2 times, most recently from 4eb4d29 to c1f90e8 Compare October 22, 2016 02:11

daxian-dbw requested changes Oct 26, 2016

View reviewed changes

daxian-dbw reviewed Oct 28, 2016

View reviewed changes

daxian-dbw reviewed Oct 31, 2016

View reviewed changes

vors force-pushed the native-pipe branch from ee87510 to 3519314 Compare November 8, 2016 02:39

daxian-dbw reviewed Nov 9, 2016

View reviewed changes

Sergei Vorobev added 5 commits November 15, 2016 14:49

Prettify NativeCommmandProcessor.cs

e038360

- Move ProcessOutputHandler outside of NativeCommmandProcessor - Remove redundent ConsumeAvailableNativeProcessOutput() blocking call

Resurect Xml serialization logic inside ProcessInputWriter

0ce50a8

Fix bug in native pipe, where we loose End() output

7914c75

Fix recursive Done() problem in steppable pipeline finalization

1508526

Trying to make 'native command lifecycle' test unflaky

32837f0

Also move it to the Feature category

vors force-pushed the native-pipe branch from 9aa165b to a620433 Compare November 15, 2016 23:14

daxian-dbw reviewed Nov 16, 2016

View reviewed changes

Separate Done() and Dispose() in ProcessInputWriter NativeCommandProc…

825352f

…essor.cs

vors force-pushed the native-pipe branch from a620433 to 825352f Compare November 16, 2016 01:16

daxian-dbw reviewed Nov 16, 2016

View reviewed changes

Remove over-optimization in NativeCommandProcessor.DequeueProcessOutput

4868cee

vors force-pushed the native-pipe branch from e55ebbe to 4868cee Compare November 16, 2016 01:44

daxian-dbw approved these changes Nov 16, 2016

View reviewed changes

daxian-dbw merged commit 8010117 into PowerShell:master Nov 16, 2016

vors mentioned this pull request Nov 29, 2016

Misleading error message when piping native command to throwing powershell #2803

Closed

joeyaiello mentioned this pull request Jan 25, 2017

PowerShell 6.0 Plan #3046

Closed

vors deleted the native-pipe branch February 9, 2017 05:19

lzybkr mentioned this pull request Feb 11, 2017

Need better way to work with native exes that report progress on stderr #3132

Closed

masaeedu mentioned this pull request Jun 7, 2017

Don’t parse the pipeline as text when it is directed from an EXE to another EXE or file. Keep the bytes as-is. #1908

Closed

daxian-dbw mentioned this pull request Aug 17, 2017

Make PowerShell Core enumerate COM collections #4553

Merged

iSazonov mentioned this pull request Feb 12, 2018

Unpredictable ordering of native command output when merging the error stream into the success output stream #5424

Closed

iSazonov mentioned this pull request Apr 27, 2018

Remove 'more' function and move the $env:PAGER capability into the help function #6059

Merged

7 tasks

nazar554 mentioned this pull request Sep 20, 2019

OpenZFS: PowerShell pipes and redirect openzfsonwindows/ZFSin#165

Open

daxian-dbw mentioned this pull request Aug 3, 2022

Data passed through a pipe will be parsed differently in PowerShell 5.1 vs current release #17523

Closed

5 tasks

		@@ -0,0 +1,50 @@
		# Minishell is a powershell concept.
		# It's primare use-case is when somebody executes a scriptblock in the new powershell process.

Native pipe #2450

Native pipe #2450

Conversation

vors commented Oct 11, 2016 • edited Loading

msftclas commented Oct 11, 2016

daxian-dbw commented Oct 19, 2016

vors commented Oct 19, 2016 • edited Loading

vors commented Oct 22, 2016

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

daxian-dbw Oct 26, 2016 • 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

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

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

vors commented Oct 27, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vors commented Nov 8, 2016

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

vors commented Oct 11, 2016 •

edited

Loading

vors commented Oct 19, 2016 •

edited

Loading

daxian-dbw Oct 26, 2016 •

edited

Loading