Simplify the logic for NamedPipeStreamReader and fix handling of multiple sequential messages

[jamill]

Update the NamedPipeStreamReader to read individual bytes from the underlying stream. This avoids complexity associated with reading a larger chunk of bytes and handling the different cases associated with that approach. This change also fixes handling of multiple sequential messages (#333)

[sanoursa]

This version is much cleaner and easier to validate. If the perf is good, this is a better way to go, I think.

[sanoursa]

Mark this readonly now that we expect it to be a fixed 1-byte array forever

[sanoursa]

Nit: this sounds like the name of a delegate. Rename to currentByte?

[sanoursa]

This method follows the Try pattern. Rename it to TryReadByte

[sanoursa]

I think you can drop this condition and always execute line 73. The caller won't use the value if we return false, so there's no point in doing the check twice.

[wilbaker]

Approved with suggestions from Saeed

[jeschu1]

I'm just curious why we don't specify a size at the start of the message. That should make it simple to read and performant.

[jamill]

I'm just curious why we don't specify a size at the start of the message.

I think that would be another fine option. I didn't go with it because it would require more changes to different components - we have native and managed components that communicate over the named pipe.

The original problem was that the existing design did not handle messages with new line characters - performance was not a problem. The thought was that it would be better to just switch out the byte that represented the end of a message, rather than deeper changes to the protocol.

[jamill]

Here is a table of data from the perf runs. A bit noisy, but I don't see any trends that indicate the simple approach introduces performance concerns.

master branch
complex version (#341)
simple version (#343)

Command	Master	Complex (PR #341)	Simple (PR #343)		Complex vs master	Simple vs Master
Unmounting old enlistment	N/A	N/A	N/A
Deleting old enlistment	N/A	N/A	N/A
gvfs version	N/A	N/A	N/A
git version	N/A	N/A	N/A
Cloning OS	18.2	19.1	19.64		-4.95%	-7.91%
gvfs mount	13.59	13.45	13.43		1.03%	1.18%
git config user.name "GVFS Perf Tests"	N/A	N/A	N/A
git config user.email "PerfTest@gvfs.com"	N/A	N/A	N/A
git config advice.statusUoption false	N/A	N/A	N/A
gvfs prefetch --folders tools;onecoreuap\inetcore\edgehtml	23.46	22.95	23.32		2.17%	0.60%
git status	0.26	0.25	0.25		3.85%	3.85%
git status	0.24	0.24	0.24		0.00%	0.00%
git status	0.24	0.24	0.24		0.00%	0.00%
Reading files	66.01	60.71	57.56		8.03%	12.80%
git status	0.26	0.25	0.25		3.85%	3.85%
git status	0.25	0.27	0.24		-8.00%	4.00%
git status	0.24	0.25	0.25		-4.17%	-4.17%
Reading files	31.24	31.67	31.37		-1.38%	-0.42%
git status	0.28	0.27	0.25		3.57%	10.71%
git status	0.25	0.24	0.24		4.00%	4.00%
git status	0.25	0.25	0.24		0.00%	4.00%
Creating new file	N/A	N/A	N/A
Editing files	N/A	N/A	N/A
git status	1.53	1.52	1.53		0.65%	0.00%
git status	1.64	1.64	1.63		0.00%	0.61%
git status	1.64	1.66	1.68		-1.22%	-2.44%
git checkout -b user/me/topic feature/gvfs/perftest/rsmaster~1	20.99	21.09	21.57		-0.48%	-2.76%
git status	1.65	1.65	6.49		0.00%	-293.33%
git status	1.73	1.69	1.70		2.31%	1.73%
git status	1.72	1.67	1.72		2.91%	0.00%
git branch -vv	0.45	0.45	0.44		0.00%	2.22%
git status	0.28	0.97	0.28		-246.43%	0.00%
git status	0.25	0.26	0.25		-4.00%	0.00%
git status	0.23	0.26	0.23		-13.04%	0.00%
git add --all	2.43	2.46	2.41		-1.23%	0.82%
git status	1.93	1.94	1.82		-0.52%	5.70%
git status	1.63	1.63	1.63		0.00%	0.00%
git status	1.67	1.70	1.62		-1.80%	2.99%
git -c core.abbrev=40 commit -m "Modified some files"	3.25	3.13	3.10		3.69%	4.62%
git status	1.94	1.81	1.98		6.70%	-2.06%
git status	1.62	1.65	1.67		-1.85%	-3.09%
git status	1.70	1.69	1.71		0.59%	-0.59%
git merge feature/gvfs/perftest/rsmaster --no-commit	20.40	20.04	20.27		1.76%	0.64%
git status --no-renames	1.63	1.62	1.63		0.61%	0.00%
git status --no-renames	1.66	1.64	1.63		1.20%	1.81%
git status --no-renames	1.65	1.63	1.63		1.21%	1.21%
git merge --abort	16.00	16.33	16.02		-2.06%	-0.12%
git status	1.67	1.64	1.66		1.80%	0.60%
git status	0.29	0.28	0.29		3.45%	0.00%
git status	0.28	0.28	0.28		0.00%	0.00%
git rebase feature/gvfs/perftest/rsmaster	27.21	27.35	27.42		-0.51%	-0.77%
git status	1.53	1.53	1.53		0.00%	0.00%
git status	1.64	1.63	1.63		0.61%	0.61%
git status	1.69	1.69	1.70		0.00%	-0.59%
git checkout feature/gvfs/perftest/rsmaster	21.18	20.99	21.03		0.90%	0.71%
git status	1.61	1.62	1.61		-0.62%	0.00%
git status	1.61	1.63	1.63		-1.24%	-1.24%
git status	1.70	1.72	1.72		-1.18%	-1.18%
gvfs prefetch --commits	3.16	2.82	3.34		10.76%	-5.70%
git fetch origin feature/gvfs/perftest/rs_es_scm	6.36	6.68	7.35		-5.03%	-15.57%
git status	0.30	0.29	0.30		3.33%	0.00%
git status	0.28	0.28	0.28		0.00%	0.00%
git status	0.24	0.24	0.24		0.00%	0.00%
git checkout feature/gvfs/perftest/rs_es_scm	21.39	21.62	21.41		-1.08%	-0.09%
git status	1.62	1.64	1.63		-1.23%	-0.62%
git status	1.70	1.71	1.72		-0.59%	-1.18%
git status	1.72	1.72	1.70		0.00%	1.16%
git checkout -b pullTest feature/gvfs/perftest/rs_es_scm~10	21.24	21.51	21.30		-1.27%	-0.28%
git pull --ff-only origin feature/gvfs/perftest/rs_es_scm	24.54	24.76	25.11		-0.90%	-2.32%
git status --no-deserialize	1.67	1.72	1.71		-2.99%	-2.40%
git status --no-deserialize	1.70	1.75	1.73		-2.94%	-1.76%
git status --no-deserialize	1.70	1.70	1.71		0.00%	-0.59%
Create and delete files	26.73	26.84	26.69		-0.41%	0.15%
git status --no-deserialize	1.49	1.50	1.52		-0.67%	-2.01%
git status --no-deserialize	1.63	1.65	1.63		-1.23%	0.00%
git status --no-deserialize	1.67	1.69	1.67		-1.20%	0.00%
Create, rename, and delete files	30.27	30.49	30.43		-0.73%	-0.53%
git status --no-deserialize	1.53	1.52	1.53		0.65%	0.00%
git status --no-deserialize	1.71	1.64	1.68		4.09%	1.75%
git status --no-deserialize	1.69	1.66	1.67		1.78%	1.18%
Unmounting	1.54	1.54	1.57		0.00%	-1.95%

[sanoursa]

@jamill thanks for the perf numbers. In almost all cases, it actually appears that the simple solution is faster than the complex one. Definitely looks good to go.