You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/05/04 19:57:32 UTC
[GitHub] [arrow] rymurr opened a new pull request #7101: [Java] ARROW-8695: Remove references to PlatformDependent in arrow-memory
rymurr opened a new pull request #7101:
URL: https://github.com/apache/arrow/pull/7101
As part of ARROW-8230 we are reducing the usages of Netty inside the arrow-memory module. This step simply removes Netty utils references
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on a change in pull request #7101: ARROW-8695: [Java] Remove references to PlatformDependent in arrow-memory
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7101:
URL: https://github.com/apache/arrow/pull/7101#discussion_r425552223
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/util/hash/SimpleHasher.java
##########
@@ -58,21 +56,21 @@ public int hashCode(long address, long length) {
int hashValue = 0;
int index = 0;
while (index + 8 <= length) {
- long longValue = getLong(address + index);
+ long longValue = MemoryUtil.UNSAFE.getLong(address + index);
int longHash = getLongHashCode(longValue);
hashValue = combineHashCode(hashValue, longHash);
index += 8;
}
if (index + 4 <= length) {
- int intValue = getInt(address + index);
+ int intValue = MemoryUtil.UNSAFE.getInt(address + index);
int intHash = intValue;
hashValue = combineHashCode(hashValue, intHash);
index += 4;
}
while (index < length) {
- byte byteValue = getByte(address + index);
+ byte byteValue = MemoryUtil.UNSAFE.getByte(address + index);
Review comment:
@jacques-n any more concerns here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7101: ARROW-8695: [Java] Remove references to PlatformDependent in arrow-memory
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7101:
URL: https://github.com/apache/arrow/pull/7101#discussion_r422074255
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/util/hash/SimpleHasher.java
##########
@@ -58,21 +56,21 @@ public int hashCode(long address, long length) {
int hashValue = 0;
int index = 0;
while (index + 8 <= length) {
- long longValue = getLong(address + index);
+ long longValue = MemoryUtil.UNSAFE.getLong(address + index);
int longHash = getLongHashCode(longValue);
hashValue = combineHashCode(hashValue, longHash);
index += 8;
}
if (index + 4 <= length) {
- int intValue = getInt(address + index);
+ int intValue = MemoryUtil.UNSAFE.getInt(address + index);
int intHash = intValue;
hashValue = combineHashCode(hashValue, intHash);
index += 4;
}
while (index < length) {
- byte byteValue = getByte(address + index);
+ byte byteValue = MemoryUtil.UNSAFE.getByte(address + index);
Review comment:
A cursory look at the bytecode shows the `PlatformDependent` call results in a few static method invocations (`INVOKESTATIC` bytecode op):
`PlatformDependent.getByte` is defined as
``` java
public static byte getByte(long address) {
return PlatformDependent0.getByte(address);
}
```
and `PlatformDependent0` is
``` java
public static byte getByte(long address) {
return UNSAFE.getByte(address);
}
```
And the `MemoryUtil` one the same with 1 less layer of indirect static calls. I would anticipate the JIT would optimise the paths to be the same.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #7101: ARROW-8695: [Java] Remove references to PlatformDependent in arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #7101:
URL: https://github.com/apache/arrow/pull/7101#issuecomment-624408406
@rymurr Thanks a lot for doing this.
It seems the title of PR does not match the title of the JIRA. Is this work still on-going? (I do not see the NettyUtils class.)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] asfgit closed pull request #7101: ARROW-8695: [Java] Remove references to PlatformDependent in arrow-memory
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #7101:
URL: https://github.com/apache/arrow/pull/7101
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7101: ARROW-8695: [Java] Remove references to PlatformDependent in arrow-memory
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7101:
URL: https://github.com/apache/arrow/pull/7101#issuecomment-623682441
https://issues.apache.org/jira/browse/ARROW-8695
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7101: ARROW-8695: [Java] Remove references to PlatformDependent in arrow-memory
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7101:
URL: https://github.com/apache/arrow/pull/7101#discussion_r422074255
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/util/hash/SimpleHasher.java
##########
@@ -58,21 +56,21 @@ public int hashCode(long address, long length) {
int hashValue = 0;
int index = 0;
while (index + 8 <= length) {
- long longValue = getLong(address + index);
+ long longValue = MemoryUtil.UNSAFE.getLong(address + index);
int longHash = getLongHashCode(longValue);
hashValue = combineHashCode(hashValue, longHash);
index += 8;
}
if (index + 4 <= length) {
- int intValue = getInt(address + index);
+ int intValue = MemoryUtil.UNSAFE.getInt(address + index);
int intHash = intValue;
hashValue = combineHashCode(hashValue, intHash);
index += 4;
}
while (index < length) {
- byte byteValue = getByte(address + index);
+ byte byteValue = MemoryUtil.UNSAFE.getByte(address + index);
Review comment:
A cursory look at the bytecode shows the `PlatformDependent` call results in a few static method invocations:
`PlatformDependent.getByte` is defined as
``` java
public static byte getByte(long address) {
return PlatformDependent0.getByte(address);
}
```
and `PlatformDependent0` is
``` java
public static byte getByte(long address) {
return UNSAFE.getByte(address);
}
```
And the MemoryUtil one the same with 1 less layer of indirect static calls. I would anticipate the JIT would optimise the paths to be the same.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7101: ARROW-8695: [Java] Remove references to PlatformDependent in arrow-memory
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7101:
URL: https://github.com/apache/arrow/pull/7101#discussion_r422074255
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/util/hash/SimpleHasher.java
##########
@@ -58,21 +56,21 @@ public int hashCode(long address, long length) {
int hashValue = 0;
int index = 0;
while (index + 8 <= length) {
- long longValue = getLong(address + index);
+ long longValue = MemoryUtil.UNSAFE.getLong(address + index);
int longHash = getLongHashCode(longValue);
hashValue = combineHashCode(hashValue, longHash);
index += 8;
}
if (index + 4 <= length) {
- int intValue = getInt(address + index);
+ int intValue = MemoryUtil.UNSAFE.getInt(address + index);
int intHash = intValue;
hashValue = combineHashCode(hashValue, intHash);
index += 4;
}
while (index < length) {
- byte byteValue = getByte(address + index);
+ byte byteValue = MemoryUtil.UNSAFE.getByte(address + index);
Review comment:
A cursory look at the bytecode shows the `PlatformDependent` call results in a few static method invocations:
`PlatformDependent.getByte` is defined as
``` java
public static byte getByte(long address) {
return PlatformDependent0.getByte(address);
}
```
and `PlatformDependent0` is
``` java
public static byte getByte(long address) {
return UNSAFE.getByte(address);
}
```
And the `MemoryUtil` one the same with 1 less layer of indirect static calls. I would anticipate the JIT would optimise the paths to be the same.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on pull request #7101: ARROW-8695: [Java] Remove references to PlatformDependent in arrow-memory
Posted by GitBox <gi...@apache.org>.
kiszk commented on pull request #7101:
URL: https://github.com/apache/arrow/pull/7101#issuecomment-623868378
Looks good to me
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7101: [Java] ARROW-8695: Remove references to PlatformDependent in arrow-memory
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7101:
URL: https://github.com/apache/arrow/pull/7101#issuecomment-623675866
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #7101: ARROW-8695: [Java] Remove references to PlatformDependent in arrow-memory
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #7101:
URL: https://github.com/apache/arrow/pull/7101#issuecomment-624595956
> > @rymurr Thanks a lot for doing this.
> > It seems the title of PR does not match the title of the JIRA. Is this work still on-going? (I do not see the NettyUtils class.)
>
> Hey @liyafan82 I have updated the JIRA title. The PR should be ready to go.
@rymurr Thanks for the effort.
There are some other references to the PlatformDependent library (e.g. class BitVectorHelper). Do you want to change them too?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on pull request #7101: ARROW-8695: [Java] Remove references to PlatformDependent in arrow-memory
Posted by GitBox <gi...@apache.org>.
rymurr commented on pull request #7101:
URL: https://github.com/apache/arrow/pull/7101#issuecomment-624596690
> > > @rymurr Thanks a lot for doing this.
> > > It seems the title of PR does not match the title of the JIRA. Is this work still on-going? (I do not see the NettyUtils class.)
> >
> >
> > Hey @liyafan82 I have updated the JIRA title. The PR should be ready to go.
>
> @rymurr Thanks for the effort.
> There are some other references to the PlatformDependent library (e.g. class BitVectorHelper). Do you want to change them too?
Hey @liyafan82 the goal of this PR is to remove from only arrow-memory. Will move to arrow-vector after ARROW-8230
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jacques-n commented on a change in pull request #7101: ARROW-8695: [Java] Remove references to PlatformDependent in arrow-memory
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #7101:
URL: https://github.com/apache/arrow/pull/7101#discussion_r421884695
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/util/hash/SimpleHasher.java
##########
@@ -58,21 +56,21 @@ public int hashCode(long address, long length) {
int hashValue = 0;
int index = 0;
while (index + 8 <= length) {
- long longValue = getLong(address + index);
+ long longValue = MemoryUtil.UNSAFE.getLong(address + index);
int longHash = getLongHashCode(longValue);
hashValue = combineHashCode(hashValue, longHash);
index += 8;
}
if (index + 4 <= length) {
- int intValue = getInt(address + index);
+ int intValue = MemoryUtil.UNSAFE.getInt(address + index);
int intHash = intValue;
hashValue = combineHashCode(hashValue, intHash);
index += 4;
}
while (index < length) {
- byte byteValue = getByte(address + index);
+ byte byteValue = MemoryUtil.UNSAFE.getByte(address + index);
Review comment:
stupid question. Are these the same bytecode once optimized. I don't know the specifics of JIT when referring to static fields.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on pull request #7101: ARROW-8695: [Java] Remove references to PlatformDependent in arrow-memory
Posted by GitBox <gi...@apache.org>.
rymurr commented on pull request #7101:
URL: https://github.com/apache/arrow/pull/7101#issuecomment-624491104
> @rymurr Thanks a lot for doing this.
> It seems the title of PR does not match the title of the JIRA. Is this work still on-going? (I do not see the NettyUtils class.)
Hey @liyafan82 I have updated the JIRA title. The PR should be ready to go.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jacques-n commented on a change in pull request #7101: ARROW-8695: [Java] Remove references to PlatformDependent in arrow-memory
Posted by GitBox <gi...@apache.org>.
jacques-n commented on a change in pull request #7101:
URL: https://github.com/apache/arrow/pull/7101#discussion_r434964912
##########
File path: java/memory/src/main/java/org/apache/arrow/memory/util/hash/SimpleHasher.java
##########
@@ -58,21 +56,21 @@ public int hashCode(long address, long length) {
int hashValue = 0;
int index = 0;
while (index + 8 <= length) {
- long longValue = getLong(address + index);
+ long longValue = MemoryUtil.UNSAFE.getLong(address + index);
int longHash = getLongHashCode(longValue);
hashValue = combineHashCode(hashValue, longHash);
index += 8;
}
if (index + 4 <= length) {
- int intValue = getInt(address + index);
+ int intValue = MemoryUtil.UNSAFE.getInt(address + index);
int intHash = intValue;
hashValue = combineHashCode(hashValue, intHash);
index += 4;
}
while (index < length) {
- byte byteValue = getByte(address + index);
+ byte byteValue = MemoryUtil.UNSAFE.getByte(address + index);
Review comment:
no, looks good.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org