You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fury.apache.org by "pushrsp (via GitHub)" <gi...@apache.org> on 2024/04/03 08:19:48 UTC
[I] Suggestion for some refactorying for MemoryBuffer class. [incubator-fury]
pushrsp opened a new issue, #1457:
URL: https://github.com/apache/incubator-fury/issues/1457
Can i do some refactor for `MemoryBuffer` class?
e.g
```java
public void writeBoolean(boolean value) {
final int writerIdx = writerIndex;
final int newIdx = writerIdx + 1; <- we can replace this number to static variable which make more understandable.
ensure(newIdx);
final long pos = address + writerIdx;
UNSAFE.putByte(heapMemory, pos, (byte) (value ? 1 : 0));
writerIndex = newIdx;
}
```
```java
public byte readByte() {
int readerIdx = readerIndex;
// every readX method use this for checking bound, so we can make short for it.
if (BoundsChecking.BOUNDS_CHECKING_ENABLED && readerIdx > size - 1) {
throw new IndexOutOfBoundsException(
String.format(
"readerIndex(%d) + length(%d) exceeds size(%d): %s", readerIdx, 1, size, this));
}
readerIndex = readerIdx + 1;
return UNSAFE.getByte(heapMemory, address + readerIdx);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [I] Suggestion for some refactorying for MemoryBuffer class. [incubator-fury]
Posted by "pushrsp (via GitHub)" <gi...@apache.org>.
pushrsp commented on issue #1457:
URL: https://github.com/apache/incubator-fury/issues/1457#issuecomment-2033992943
> Regarding the second suggestion, how to simplify it? Extract a function separately for the checked logic and call it in readXXX?
i would simplify like this below.
```java
public byte readByte() {
int readerIdx = readerIndex;
throwIfOutOfBound(readerIdx, 1);
readerIndex = readerIdx + 1;
return UNSAFE.getByte(heapMemory, address + readerIdx);
}
public short readShort() {
int readerIdx = readerIndex;
throwIfOutOfBound(readerIdx, 2)
readerIndex = readerIdx + 2;
final long pos = address + readerIdx;
if (LITTLE_ENDIAN) {
return UNSAFE.getShort(heapMemory, pos);
} else {
return Short.reverseBytes(UNSAFE.getShort(heapMemory, pos));
}
}
...
private void throwIfOutOfBound(int readerIdx, int dataSize) {
if(BoundsChecking.BOUNDS_CHECKING_ENABLED && readerIdx > size - dataSize) {
throw new IndexOutOfBoundsException(
String.format(
"readerIndex(%d) + length(%d) exceeds size(%d): %s", readerIdx, dataSize, size, this));
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [I] Suggestion for some refactorying for MemoryBuffer class. [incubator-fury]
Posted by "LiangliangSui (via GitHub)" <gi...@apache.org>.
LiangliangSui commented on issue #1457:
URL: https://github.com/apache/incubator-fury/issues/1457#issuecomment-2033966875
Hi @pushrsp ，thanks for your suggession.
Regarding your first proposal, I think it is unnecessary, we use one byte to store the value of boolean type. We can also know it from the code `UNSAFE.putByte(heapMemory, pos, (byte) (value ? 1 : 0));` below, so `writerIdx + 1` itself is easy to understand.
Regarding the second suggestion, how to simplify it? Extract a function separately for the checked logic and call it in readXXX?
cc @chaokunyang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [I] Suggestion for some refactorying for MemoryBuffer class. [incubator-fury]
Posted by "chaokunyang (via GitHub)" <gi...@apache.org>.
chaokunyang commented on issue #1457:
URL: https://github.com/apache/incubator-fury/issues/1457#issuecomment-2034404847
I refactorized this code, it has been replaced by a `StreamReader` in pr #1451 .
For changes here, seems throwIfOutOfBound will be invoked every time. The method invocation is not ignorable in such primitives. Jvm inline doesn't happens sometimes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [I] Suggestion for some refactorying for MemoryBuffer class. [incubator-fury]
Posted by "LiangliangSui (via GitHub)" <gi...@apache.org>.
LiangliangSui commented on issue #1457:
URL: https://github.com/apache/incubator-fury/issues/1457#issuecomment-2034053238
LGTM.
We don't have to worry about `throwIfOutOfBound` function not being inlined because `throwIfOutOfBound` is rarely executed.
BTW, we can also use [throwIndexOutOfBoundsException function](https://github.com/apache/incubator-fury/blob/main/java/fury-core/src/main/java/org/apache/fury/memory/MemoryBuffer.java#L1866-L1870) to replace `throw new IndexOutOfBoundsException(...)`.
What do you think about this suggestion? @chaokunyang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org
Re: [I] Suggestion for some refactorying for MemoryBuffer class. [incubator-fury]
Posted by "pushrsp (via GitHub)" <gi...@apache.org>.
pushrsp closed issue #1457: Suggestion for some refactorying for MemoryBuffer class.
URL: https://github.com/apache/incubator-fury/issues/1457
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fury.apache.org
For additional commands, e-mail: commits-help@fury.apache.org