Use a once-allocated buffer for reading native pixel values instead of using binary.Read (which allocates a slice under the hood) #163

kaxap · 2020-12-20T01:06:20Z

addressing #161

catching up with upstream

…the Reader

kaxap · 2020-12-20T01:25:42Z

Benchmarks looking nice

ReadNativeFrames/10x10,_10_frames,_1_sample/pixel-2    1.89µs ± 6%  2.32µs ± 3%  +23.20%  (p=0.000 n=8+8)
ReadNativeFrames/100x100,_10_frames,_1_sample/pixel-2  86.1µs ± 2%  83.9µs ± 2%   -2.56%  (p=0.002 n=7+7)
ReadNativeFrames/512x512,_10_frames,_1_sample/pixel-2  2.34ms ± 3%  1.65ms ± 6%  -29.48%  (p=0.000 n=8+8)
ReadNativeFrames/512x512,_10_frames,_5_sample/pixel-2   814ms ± 6%     4ms ± 7%  -99.51%  (p=0.000 n=8+8)
Parse/1.dcm-2                                          2.62ms ± 3%  1.33ms ± 2%  -49.34%  (p=0.000 n=8+8)
Parse/2.dcm-2                                          2.69ms ± 3%  1.32ms ± 4%  -50.96%  (p=0.001 n=7+7)
Parse/3.dcm-2                                          2.53ms ± 3%  1.19ms ± 2%  -52.91%  (p=0.000 n=8+8)
Parse/4.dcm-2                                           268ms ± 7%   116ms ± 3%  -56.75%  (p=0.000 n=8+8)
Parse/5.dcm-2                                          38.2ms ± 2%  17.7ms ± 2%  -53.79%  (p=0.000 n=7+8)

suyashkumar

Thank you @kaxap for the contribution! The improvement looks great from avoiding binary.Read which allocates a slice each time and instead manually unpacking the bytes into the right uint by reusing a once-allocated buffer.

Generally looks good, however I had some comments for you to consider before I merge.

Thanks again for the improvement :D

pkg/dicomio/reader.go

read.go

suyashkumar · 2020-12-20T05:20:27Z

read.go

+				}
+				if n < bitsAllocated/8 {
+					return nil, bytesRead,
+						fmt.Errorf("not enough bytes in the input to read uint%d", bitsAllocated)


just a thought: can we create a sentinel error for this (e.g ErrorIncompleteRead or something) and wrap that error with the right bitsAllocated info?

also, could we maybe add a test case here to trigger this and expect we get this type of error? we would have to update the test to use errors.Is. Also, I can send a change for this later too if we prefer.

pkg/dicomio/reader.go

mocks/pkg/dicomio/mock_reader.go

Co-authored-by: Suyash Kumar <suyashkumar2003@gmail.com>

read.go

Add test for ErrorIncompleteRead minor refactoring

kaxap · 2020-12-20T19:14:22Z

@suyashkumar appreciate your suggestions, implemented all of them I believe.

suyashkumar

Thanks so much for making those updates! I think it's looking pretty much good to go, I just had one final set of thoughts for you to consider and then let's get this merged. Thank you again for the contribution 😄

suyashkumar · 2020-12-21T05:01:20Z

read.go

-					val, err := d.ReadUInt8()
-					if err != nil {
-						return nil, bytesRead, err
+


nit: no need for a new line here (but I can also just fix in a cleanup pr later, don't need to block this).

Suggested change

suyashkumar · 2020-12-21T05:11:20Z

read.go

-					if err != nil {
-						return nil, bytesRead, err
+
+				n, err := d.Read(pixelBuf)


Thought about this a little more the second time around (sorry for not bringing this up earlier). Looking at the io.Reader interface I think a single call to read may read "up to len(p)" bytes. Perhaps we should use io.ReadFull to try to read at least the size of the buffer.

In reality for values like 16 or 32 bits, calls to Read may still work no problem, but it may be more "correct" to use something like io.ReadFull which won't be expensive in the case that Read would've worked on the first try. This may also insulate us from the exact implementation of the reader under the hood we're reading from, since there is no garuntee that it must fill the buffer on the first call to Read (but it's possible it may fill the buffer correctly on subsequent calls to Read--see the implementation of ReadAtLeast).

It also looks like based on the implementation, it may return a nice ErrUnexpectedEOF if it reads less than n bytes, saving us an error, but we can leave it in there for not and not depend on that to make it easy and get this merged for now.

WDYT?

Suggested change

n, err := d.Read(pixelBuf)

n, err := io.ReadFull(d, pixelBuf)

Good catch. Read(buffer) can in fact return less than n bytes when reading from a network socket, which is expected behaviour.

read.go

Remove ErrorIncompleteRead as a consequence Update ReadNativeFrames test accordingly

suyashkumar

Looks good to me, thanks for making the suggested updates and thanks for the contribution!

suyashkumar · 2020-12-22T04:41:20Z

🎉 merged!

kaxap and others added 3 commits December 20, 2020 06:41

Merge pull request #1 from suyashkumar/main

1780bba

catching up with upstream

pre-allocate pixel buffer in readNativeFrames; add GetByteOrder() to …

7883858

…the Reader

add GetByteOrder() to the MockReader

1e175d1

suyashkumar self-requested a review December 20, 2020 05:08

suyashkumar self-assigned this Dec 20, 2020

suyashkumar added enhancement New feature or request performance labels Dec 20, 2020

suyashkumar reviewed Dec 20, 2020

View reviewed changes

pkg/dicomio/reader.go Outdated Show resolved Hide resolved

suyashkumar reviewed Dec 20, 2020

View reviewed changes

mocks/pkg/dicomio/mock_reader.go Outdated Show resolved Hide resolved

kaxap and others added 4 commits December 20, 2020 16:03

Update pkg/dicomio/reader.go

e423725

Co-authored-by: Suyash Kumar <suyashkumar2003@gmail.com>

Update read.go

7f107ef

Co-authored-by: Suyash Kumar <suyashkumar2003@gmail.com>

Update pkg/dicomio/reader.go

3ef4870

Co-authored-by: Suyash Kumar <suyashkumar2003@gmail.com>

Update mocks/pkg/dicomio/mock_reader.go

fb1393c

Co-authored-by: Suyash Kumar <suyashkumar2003@gmail.com>

kaxap commented Dec 20, 2020

View reviewed changes

read.go Outdated Show resolved Hide resolved

kaxap and others added 2 commits December 20, 2020 16:09

Update read.go

9e5de7d

Add sentinel ErrorIncompleteRead for readNativeFrames

25288ff

Add test for ErrorIncompleteRead minor refactoring

suyashkumar reviewed Dec 21, 2020

View reviewed changes

suyashkumar changed the title ~~pre-allocate buffer for reading pixel data in readNativeFrames~~ Use a manual once-allocated buffer for reading pixel values instead of using binary.Read (which allocates a slice under the hood) Dec 21, 2020

Update ReadNativeFrames to use io.ReadFull instead of Read

075bb74

Remove ErrorIncompleteRead as a consequence Update ReadNativeFrames test accordingly

suyashkumar approved these changes Dec 22, 2020

View reviewed changes

suyashkumar merged commit a6f29f2 into suyashkumar:main Dec 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a once-allocated buffer for reading native pixel values instead of using binary.Read (which allocates a slice under the hood) #163

Use a once-allocated buffer for reading native pixel values instead of using binary.Read (which allocates a slice under the hood) #163

kaxap commented Dec 20, 2020 •

edited

Loading

kaxap commented Dec 20, 2020

suyashkumar left a comment •

edited

Loading

suyashkumar Dec 20, 2020

suyashkumar Dec 20, 2020

kaxap Dec 20, 2020

kaxap commented Dec 20, 2020

suyashkumar left a comment

suyashkumar Dec 21, 2020

suyashkumar Dec 21, 2020 •

edited

Loading

kaxap Dec 21, 2020 •

edited

Loading

suyashkumar left a comment

suyashkumar commented Dec 22, 2020

	n, err := d.Read(pixelBuf)
	n, err := io.ReadFull(d, pixelBuf)

Use a once-allocated buffer for reading native pixel values instead of using binary.Read (which allocates a slice under the hood) #163

Use a once-allocated buffer for reading native pixel values instead of using binary.Read (which allocates a slice under the hood) #163

Conversation

kaxap commented Dec 20, 2020 • edited Loading

kaxap commented Dec 20, 2020

suyashkumar left a comment • edited Loading

Choose a reason for hiding this comment

suyashkumar Dec 20, 2020

Choose a reason for hiding this comment

suyashkumar Dec 20, 2020

Choose a reason for hiding this comment

kaxap Dec 20, 2020

Choose a reason for hiding this comment

kaxap commented Dec 20, 2020

suyashkumar left a comment

Choose a reason for hiding this comment

suyashkumar Dec 21, 2020

Choose a reason for hiding this comment

suyashkumar Dec 21, 2020 • edited Loading

Choose a reason for hiding this comment

kaxap Dec 21, 2020 • edited Loading

Choose a reason for hiding this comment

suyashkumar left a comment

Choose a reason for hiding this comment

suyashkumar commented Dec 22, 2020

kaxap commented Dec 20, 2020 •

edited

Loading

suyashkumar left a comment •

edited

Loading

suyashkumar Dec 21, 2020 •

edited

Loading

kaxap Dec 21, 2020 •

edited

Loading