Skip to content

Conversation

@xerial
Copy link
Member

@xerial xerial commented May 30, 2014

Optimized unpackBinary by reducing the memory copy in MessageBufferInput implementations.

The other changes are as follows:

  • Added test cases for reading from MessageBufferInput implementations.
  • Added a test case for comparing unpackBinary performance between v06 and v07.
  • Fixed several bugs

A performance report is in #103.

@xerial xerial added this to the v07 milestone May 30, 2014
@xerial xerial changed the title Optimize unpackBinary #103 Optimize unpackBinary May 30, 2014
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to check offset + length <= arr.length?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. I will fix this statement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about checking here if the sum of offset + length doesn't exceed size?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. If that test fails this code should throw IllegalArgumentException.

@komamitsu
Copy link
Member

👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instanceof ByteArrayInputStream is not safe because in could be an instance of extended type of ByteArrayInputStream which overrides read(byte[]) method.
This is safe: ByteArrayInputStream.class.equals(in.getClass())

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. We should compare the exact class type.

xerial added a commit that referenced this pull request Jun 4, 2014
@xerial xerial merged commit f53e5ae into v07-unpacker Jun 4, 2014
@xerial xerial deleted the v07-unpack-binary branch June 4, 2014 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants