Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove .clone() for ByteString #18

Closed
letmaik opened this issue Dec 11, 2015 · 6 comments
Closed

Remove .clone() for ByteString #18

letmaik opened this issue Dec 11, 2015 · 6 comments

Comments

@letmaik
Copy link

letmaik commented Dec 11, 2015

My use case is to encode big byte strings (which are then tagged as typed arrays), possibly up to 100MiB. I haven't profiled it yet, but from previous experience I can smell that the ByteString class will be one source of bottlenecks due to its use of array cloning within the constructor and also in getBytes(). In the ByteStringEncoder the getBytes() method is called and there is absolutely no reason to make a copy here. For the constructor, well, I would put more responsibilty into the user's hands and explicitly say in the javadoc that no copy is made, so the user has to make one himself if he wants to further use the byte array.

Removing the cloning operations will not only reduce memory usage spikes but also remove the time it would take to make the clone and also the effort that the garbage collector would have.

@c-rack
Copy link
Owner

c-rack commented Dec 11, 2015

The call to clone() is a consequence of our goal to pass all PMD, CPD and FindBugs tests.

Unfortunately, PMD considers your suggestion as a security problem:
http://pmd.sourceforge.net/pmd-4.3.0/rules/sunsecure.html

I would merge a PR that removes the clone() call while preserving the PMD tests passing, for example by providing a proper exclusion of those tests. :-)

@letmaik
Copy link
Author

letmaik commented Dec 11, 2015

OK I understand, I will submit a pull request tomorrow or so. Thanks!

@letmaik
Copy link
Author

letmaik commented Dec 13, 2015

I just had a look at how to configure PMD and it seems to be quite tricky to exclude a rule for a specific file or even method. I was hoping for some kind of annotation but that's not how it works apparently. I'm afraid I won't have the time to look into that unfortunately.

Since the encoder is hardwired to that specific ByteString class (and not an interface) I also can't just extend or copy a modified version into my codebase as a quick fix. With an extends I would still have to use the parent constructor which does the cloning.

Any suggestion?

@c-rack
Copy link
Owner

c-rack commented Dec 13, 2015

I'll take care about that :-)

c-rack added a commit that referenced this issue Dec 14, 2015
Solution: remove the `clone()`

Fixes #18

The call to `clone()` was originally introduced to pass the PMD checks.
However, the PMD checks seem to be broken for a while, so we can safely
change this.

Additionally, we add a test that ensures that cloning will not be
re-introduced later.
@letmaik
Copy link
Author

letmaik commented Dec 14, 2015 via email

@c-rack
Copy link
Owner

c-rack commented Dec 14, 2015

Hope to find the time for an official release (also on maven central) today

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

No branches or pull requests

2 participants