Zero Copy ArrayBuffer

[Ouwen]

This PR changes the ReadBufferStream to not allocate new memory ArrayBuffers when calling .more The idea being that ReadBufferStreams can be treated as a "View" on top of some underlying ArrayBuffer sitting in memory.

• It doesn't really change the speed performance.
• I haven't done any memory profiling.

Thoughts? :+1: :-1:

[pieper]

From a quick look this seems like a nice improvement, thanks 👍

@Punzo or @hackermd or anyone could have a look to see if there are any implications for your use? Seems like this should improve memory usage on big images.

[Punzo]

I did not tested, but yes for large images definitely this would help.

[pieper]

Do you think we should test before merging? If there's a regression would it cause any problems for you?

[Punzo]

I will test this tomorrow !

[Ouwen]

Probably makes sense to remove the view option since making the DataView object is pretty negligible.

[Punzo]

tested over OHIF and Slim on a couple of studies, no regression found

[pieper]

Thank you for testing @Punzo 👍

@Ouwen I see you took out the view feature. Is everything else in place and you think this is ready to merge?

[Ouwen]

@pieper I made some changes to the buffer returns so that Uint8Array views are returned instead of new buffer slices.

The tests pass locally for me.

I did have to make a change to the ReadBufferStream to check for Uint8Array and slice an array buffer to get the DicomDict writing to work.

Additionally, it seemed like the normailzer also depended on an ArrayBuffer return instead of a Uint8Array view so performed a similar Uint8Array slice conversion.

This is a bit of a larger API change so we can make it an optional param in DicomMessage.readFile in case there is worries on a regression.

[pieper]

This is a bit of a larger API change so we can make it an optional param in DicomMessage.readFile in case there is worries on a regression.

Yes, I'm concerned about regressions in general since people are using the code in various places and lately there have been several improvement PRs, that are much appreciated but also difficult to evaluate since I don't use the code on a daily basis myself. Of course we've never released a 1.0 version so everything is technically changeable but stability would be preferred.

Making the improvements without changing the API would be preferred as long as the code doesn't end up being too ugly.

[Ouwen]

Sounds good, I'll go ahead and remove the Uint8Array view returns and stick to the original array buffer copies to keep the same API. Will rebase to the most recent master and push up

[Ouwen]

@pieper should be ready for merge

[richard-viney]

After looking at rebasing my PR #276 onto this one a few things came up:

• Previously PixelData was always returned via ArrayBuffer, but now sometimes it's Uint8Array instances (but not always). This inconsistency of return type seems undesirable? The discussion above suggests there was no intention to change the return type from ArrayBuffer, but this has still happened in the case of encapsulated data using an offset table.
• When the ReadBufferStream codepath is used, the actual parent stream being read from isn't moved forward to the end of the PixelData once BinaryRepresentation.readBytes() is done, which leads to subsequent errors/warnings about invalid VRs being reported.

I've updated my PR #276 to address both of these issues, and tighten up the parsing compliance further. I've avoided all copies where we can, given the restrictions of needing to return ArrayBuffer instances. As noted in previous comments, if we returned Uint8Array instead then we could avoid some copies, but this is a public API change that I suspect may break things in the library that don't currently have full test coverage, so I think a change to Uint8Array is best handled separately. For now it seems reasonable to stick with ArrayBuffer, avoiding copies where we can.

I've also added some additional tests around the return type of binary tags.

See #276 for details.

Thoughts?

[pieper]

Oof, thanks for bringing this up @richard-viney. These are the kind of side effects I worry about and thanks for your work to resolve them 👍

@Ouwen can you please have a look?

[Ouwen]

@richard-viney many thanks for giving this a review, will look at your PR now