Add an ImmutableByteArray implementation by markelliot · Pull Request #139 · palantir/conjure-java

markelliot · 2018-12-20T14:02:16Z

Preparation for replacing ByteBuffer in generated classes since ByteBuffer is not
immutable and maintains mutable state.

Preparation for replacing ByteBuffer in generated classes since ByteBuffer is not immutable and maintains mutable state.

uschi2000 · 2018-12-20T20:20:18Z

@j-baker Didn't you have one of those somewhere already?

carterkozak

Strongly in favor of this feature, thanks Mark!
I recall @schlosna was interested in this feature as well.

Old PR from @j-baker:<internal-github>/conjure/pull/816

carterkozak · 2018-12-21T13:11:12Z

+    }
+
+    /** Copies this byte array into the provided byte array beginning at offset and up to the provided length. */
+    public void copy(byte[] array, int offset, int length) {


would prefer to rename the array parameter something along the lines of destination

bikeshed: method name copyTo?

yep, will rename to copyTo

carterkozak · 2018-12-21T13:15:58Z

+    public byte[] getBytes() {
+        byte[] unsafe = new byte[safe.length];
+        System.arraycopy(safe, 0, unsafe, 0, safe.length);
+        return unsafe;


return safe.clone() is simpler.

I'd prefer to know we're doing this using the System copy primitives, and so going to keep this relatively small amount of code, confidently given test coverage.

carterkozak · 2018-12-21T13:17:10Z

+    }
+
+    /** Returns a new read-only {@link ByteBuffer} backed by this byte array. */
+    public ByteBuffer getByteBuffer() {


Thoughts on as or to over get?

I guess “asNewByteBuffer”?

Not sure new is necessary here, since the byte buffer isn't mutable. It happens to be new, but that makes no difference to api consumers.
Our data could just as easily be backed by a heap ByteBuffer rather than byte[], in which case we wouldn't create new objects.

Since this is a constant time/memory operation unlike getBytes the originalgetByteBuffer is probably best here.

ok, for symmetry this will be called asByteBuffer (no "new").

carterkozak · 2018-12-21T13:19:26Z

+
+    /** Returns a copy of this byte array. */
+    @JsonValue
+    public byte[] getBytes() {


It might be helpful to include some indication in the method name that we're making a copy, which is potentially expensive.

“asNewByteArray”?

carterkozak · 2018-12-21T13:22:01Z

+import java.util.Arrays;
+
+/** An immutable {@code byte[]} wrapper. */
+public final class ImmutableByteArray {


Thoughts on naming this Binary to couple this with the cojure primitive? Byte array is an implementation detail.

Maybe ImmutableBytes? “binary” translates a few different ways — sometimes as a stream — so would avoid the direct naming here. Also open to “ByteString” or just “Bytes”.

Bytes is reasonable, though I prefer Binary. Other types have different java bindings based on context, int, OptionalInt, List<Integer> for example.
Bytes sounds a bit like a utility rather than a data object (see com.google.common.primitives.Bytes).

why not just ByteArray? why don't prefix other classes with Immutable even if they are, e.g. ResourceIdentifier.

going to go with "Bytes"

carterkozak · 2018-12-21T13:24:17Z

+    public boolean equals(Object obj) {
+        return this == obj
+                || (obj instanceof ImmutableByteArray && Arrays.equals(safe, ((ImmutableByteArray) obj).safe));
+    }


Missing toString, though I'm not sure what I'd expect a binary toString to do. Perhaps class name and length?

added toString that includes the size but nothing else

carterkozak · 2018-12-21T13:31:56Z

+    }
+
+    /** Returns a copy of this byte array. */
+    @JsonValue


Should we move @JsonValue to getByteBuffer and avoid duplicating contents for serialization? Annotation may apply to the safe field, but I've not tested that type of usage.

sure, I'll move it to the other method. It's more appropriate to label a method than a private member, so we'll stick with methods.

schlosna

palantir/atlasdb#2020 was one place where we didn't want to require a 3rd party dependency in public API that this could be useful.

schlosna · 2018-12-21T14:01:18Z

+    }
+
+    /** Copies this byte array into the provided byte array beginning at offset and up to the provided length. */
+    public void copy(byte[] array, int offset, int length) {


bikeshed: method name copyTo?

uschi2000 · 2018-12-21T17:58:26Z

+
+    /** Constructs a new {@link ImmutableByteArray} read from the provided {@link ByteBuffer}. */
+    public static ImmutableByteArray from(ByteBuffer buffer) {
+        ByteBuffer local = buffer.duplicate();


can you add a comment saying why this duplicate is necessary?

uschi2000 · 2018-12-21T17:59:27Z

+        assertThat(immutable.getBytes()[0]).isEqualTo((byte) 0);
+
+        input[0] = 1;
+


nit: could cut this line (34) and the one above (30)

added a comment on what line 33 is doing, believe the whitespace improves readability

uschi2000 · 2018-12-21T18:00:10Z

+
+    @Test
+    public void testConstructionCopiesArrayWithRange() {
+        byte[] input = new byte[]{0, 1, 2};


also test end by doing {0,1,2,3} --> {1,2}?

uschi2000

looks good modulo the naming discussion and one docs suggestion.

carterkozak

thanks

Add an ImmutableByteArray implementation

da887f0

Preparation for replacing ByteBuffer in generated classes since ByteBuffer is not immutable and maintains mutable state.

markelliot requested a review from a team as a code owner December 20, 2018 14:02

Mark Elliot added 3 commits December 20, 2018 16:21

more factory methods

8fc1bb4

punctuation

e8bfb73

checkstyle

de87597

carterkozak reviewed Dec 21, 2018

View reviewed changes

schlosna reviewed Dec 21, 2018

View reviewed changes

uschi2000 reviewed Dec 21, 2018

View reviewed changes

uschi2000 approved these changes Dec 21, 2018

View reviewed changes

Mark Elliot added 2 commits January 6, 2019 16:30

address CR

ec35087

better comment

6329b59

carterkozak approved these changes Jan 6, 2019

View reviewed changes

markelliot merged commit 758f349 into develop Jan 7, 2019

markelliot deleted the me/immutableByteArray branch January 7, 2019 14:42

carterkozak mentioned this pull request Feb 6, 2019

[improvement] 'UseImmutableBytes' feature flag allows opting-in to Bytes instead of ByteBuffer #226

Merged

		assertThat(immutable.getBytes()[0]).isEqualTo((byte) 0);

		input[0] = 1;

Conversation

markelliot commented Dec 20, 2018

Uh oh!

uschi2000 commented Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carterkozak left a comment • edited by iamdanfox Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schlosna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uschi2000 left a comment

Choose a reason for hiding this comment

Uh oh!

carterkozak left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

uschi2000 commented Dec 20, 2018 •

edited

Loading

carterkozak left a comment •

edited by iamdanfox

Loading