Skip to content

Commit

Permalink
Remove short conversions in MapBuffer
Browse files Browse the repository at this point in the history
Summary:
MapBuffer uses unsigned short in C++, but Java doesn't really have a type that represents that. That means that MapBuffer would be limited to max 32768 values instead of 65536, which doesn't make much sense as a limitation.
Considering weird (and usually not performant) handling of short values in Java in general, this change replaces them with ints, converting keys from short when needed with `key & 0xFFFF`.

Changelog: [Internal]

Reviewed By: javache

Differential Revision: D33595308

fbshipit-source-id: a7adde8a207bb4aa1d81d367ab5d7b41ace2e291
  • Loading branch information
Andrei Shikov authored and facebook-github-bot committed Jan 17, 2022
1 parent ead6695 commit a054379
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 39 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -39,21 +39,10 @@ public class ReadableMapBuffer implements Iterable<ReadableMapBuffer.MapBufferEn

private static final int INT_SIZE = 4;

// TODO T83483191: consider moving short to INTs, we are doing extra cast operations just because
// of short java operates with int
private static final int SHORT_SIZE = 2;

private static final short SHORT_ONE = (short) 1;

@Nullable ByteBuffer mBuffer = null;

// Size of the Serialized Data
@SuppressWarnings("unused")
private int mSizeOfData = 0;

// Amount of items serialized on the ByteBuffer
@SuppressWarnings("unused")
private short mCount = 0;
private int mCount = 0;

@DoNotStrip
private ReadableMapBuffer(HybridData hybridData) {
Expand All @@ -72,11 +61,11 @@ private ReadableMapBuffer(ByteBuffer buffer) {
@Nullable
private HybridData mHybridData;

private int getKeyOffsetForBucketIndex(int bucketIndex) {
private static int getKeyOffsetForBucketIndex(int bucketIndex) {
return HEADER_SIZE + BUCKET_SIZE * bucketIndex;
}

private int getValueOffsetForKey(short key) {
private int getValueOffsetForKey(int key) {
importByteBufferAndReadHeader();
int bucketIndex = getBucketIndexForKey(key);
if (bucketIndex == -1) {
Expand All @@ -98,25 +87,25 @@ private int getOffsetForDynamicData() {
* @return the "bucket index" for a key or -1 if not found. It uses a binary search algorithm
* (log(n))
*/
private int getBucketIndexForKey(short key) {
short lo = 0;
short hi = (short) (getCount() - SHORT_ONE);
private int getBucketIndexForKey(int key) {
int lo = 0;
int hi = getCount() - 1;
while (lo <= hi) {
final short mid = (short) ((lo + hi) >>> SHORT_ONE);
final short midVal = readKey(getKeyOffsetForBucketIndex(mid));
final int mid = (lo + hi) >>> 1;
final int midVal = readKey(getKeyOffsetForBucketIndex(mid));
if (midVal < key) {
lo = (short) (mid + SHORT_ONE);
lo = mid + 1;
} else if (midVal > key) {
hi = (short) (mid - SHORT_ONE);
hi = mid - 1;
} else {
return mid;
}
}
return -1;
}

private short readKey(int position) {
return mBuffer.getShort(position);
private int readKey(int position) {
return mBuffer.getShort(position) & 0xFFFF;
}

private double readDoubleValue(int bufferPosition) {
Expand Down Expand Up @@ -166,9 +155,7 @@ private void readHeader() {
mBuffer.order(ByteOrder.LITTLE_ENDIAN);
}
// count
mCount = mBuffer.getShort();
// size
mSizeOfData = mBuffer.getInt();
mCount = mBuffer.getShort() & 0xFFFF;
}

/**
Expand All @@ -177,13 +164,13 @@ private void readHeader() {
* @param key Key to search for
* @return true if and only if the Key received as a parameter is stored in the MapBuffer.
*/
public boolean hasKey(short key) {
public boolean hasKey(int key) {
// TODO T83483191: Add tests
return getBucketIndexForKey(key) != -1;
}

/** @return amount of elements stored into the MapBuffer */
public short getCount() {
public int getCount() {
importByteBufferAndReadHeader();
return mCount;
}
Expand All @@ -192,7 +179,7 @@ public short getCount() {
* @param key {@link int} representing the key
* @return return the int associated to the Key received as a parameter.
*/
public int getInt(short key) {
public int getInt(int key) {
// TODO T83483191: extract common code of "get methods"
return readIntValue(getValueOffsetForKey(key));
}
Expand All @@ -201,27 +188,27 @@ public int getInt(short key) {
* @param key {@link int} representing the key
* @return return the double associated to the Key received as a parameter.
*/
public double getDouble(short key) {
public double getDouble(int key) {
return readDoubleValue(getValueOffsetForKey(key));
}

/**
* @param key {@link int} representing the key
* @return return the int associated to the Key received as a parameter.
*/
public String getString(short key) {
public String getString(int key) {
return readStringValue(getValueOffsetForKey(key));
}

public boolean getBoolean(short key) {
public boolean getBoolean(int key) {
return readBooleanValue(getValueOffsetForKey(key));
}

/**
* @param key {@link int} representing the key
* @return return the int associated to the Key received as a parameter.
*/
public ReadableMapBuffer getMapBuffer(short key) {
public ReadableMapBuffer getMapBuffer(int key) {
return readMapBufferValue(getValueOffsetForKey(key));
}

Expand All @@ -241,8 +228,8 @@ private ByteBuffer importByteBufferAndReadHeader() {
return mBuffer;
}

private void assertKeyExists(short key, int bucketIndex) {
short storedKey = mBuffer.getShort(getKeyOffsetForBucketIndex(bucketIndex));
private void assertKeyExists(int key, int bucketIndex) {
int storedKey = readKey(getKeyOffsetForBucketIndex(bucketIndex));
if (storedKey != key) {
throw new IllegalStateException(
"Stored key doesn't match parameter - expected: " + key + " - found: " + storedKey);
Expand Down Expand Up @@ -277,8 +264,8 @@ public boolean equals(@Nullable Object obj) {
@Override
public Iterator<MapBufferEntry> iterator() {
return new Iterator<MapBufferEntry>() {
short current = 0;
short last = (short) (getCount() - SHORT_ONE);
int current = 0;
final int last = getCount() - 1;

@Override
public boolean hasNext() {
Expand All @@ -301,7 +288,7 @@ private MapBufferEntry(int position) {
}

/** @return a {@link short} that represents the key of this {@link MapBufferEntry}. */
public short getKey() {
public int getKey() {
return readKey(mBucketOffset);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ private static void buildSpannableFromFragment(
SpannableStringBuilder sb,
List<SetSpanOperation> ops) {

for (short i = 0, length = fragments.getCount(); i < length; i++) {
for (int i = 0, length = fragments.getCount(); i < length; i++) {
ReadableMapBuffer fragment = fragments.getMapBuffer(i);
int start = sb.length();

Expand Down

0 comments on commit a054379

Please sign in to comment.