Improve buffer usage across bolt & packstream classes #224
Conversation
ali-ince
commented
Aug 16, 2017
ali-ince
commented
- Introduced Buffer related settings into the configuration,
- Dropped MemoryStream from ChunkReader and started to use a fix sized buffer,
- Dropped MemoryStream from BoltWriter,
- Reset MemoryStream capacity to default buffer size after consuming or sending messages in BoltReader and ChunkWriter classes.
|
Looks okay, but would like to hear more input from @lutovich |
| if (_currentChunkSize == -1 && HasBytesAvailable(_chunkSizeBuffer.Length)) | ||
| { | ||
| ReadFromChunkStream(_chunkSizeBuffer, 0, _chunkSizeBuffer.Length); | ||
| ReadFromBuffer(_chunkSizeBuffer, 0, _chunkSizeBuffer.Length); |
There was a problem hiding this comment.
This method is only used for reading chunk size, maybe we could just move all logic of reading size header into the same method and rename it as ReadChunkSize()?
| CopyToFromChunkStream(targetStream, _currentChunkSize); | ||
| var count = Math.Min(_currentChunkSize, _lastWritePosition - _lastReadPosition); | ||
|
|
||
| CopyFromBuffer(targetStream, count); |
There was a problem hiding this comment.
Will it be a good idea if we rename targetStream to messageStream/dechunkedStream and downStream to networkStream?
There was a problem hiding this comment.
Renamed targetStream to messageStream but did not touch downStream as it may not be networkStream which we're dealing with.
| _lastWritePosition += read; | ||
|
|
||
| // Can we read a whole message from what we have? | ||
| if (TryReadFromBuffer(targetStream)) |
There was a problem hiding this comment.
TryReadOneCompleteMessage?
There was a problem hiding this comment.
renamed to TryReadOneCompleteMessageFromBuffer.
|
Reviewed with @lutovich, I got a few naming suggestions. He got the question if it is needed to expose the |
1. Introduced Buffer related settings into the configuration, 2. Dropped MemoryStream from ChunkReader and started to use a fix sized buffer, 3. Dropped MemoryStream from BoltWriter, 4. Reset MemoryStream capacity to default buffer size after consuming or sending messages in BoltReader and ChunkWriter classes.
ali-ince
force-pushed
the
1.5-improve-buffer-usage
branch
from
860d59b
to
ce26507
Compare
|
Regarding config options, there are defaults set and unless the user does not have any specific needs (very large reads or very large writes) we do not expect them to set those config options. For me it is better to have config options with suitable defaults and let the user change them upon any requirement. We may discuss further if needed. |
