You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "onebox-li (via GitHub)" <gi...@apache.org> on 2023/11/09 08:26:25 UTC
[PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
onebox-li opened a new pull request, #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086
### What changes were proposed in this pull request?
Change WorkerInfo#hashCode() from map+foldLeft to while and cache.
Test the each way to calculate, code and result show as below:
```
val state = Seq(host, rpcPort, pushPort, fetchPort, replicatePort)
// origin
val originHash = state.map(_.hashCode()).foldLeft(0)((a, b) => 31 * a + b)
// for
var forHash = 0
for (i <- state) {
forHash = 31 * forHash + i.hashCode()
}
// while
var whileHash = 0
var i = 0
while (i < state.size) {
whileHash = 31 * whileHash + state(i).hashCode()
i = i + 1
}
```
Result:
```
java version "1.8.0_261"
origin hash result = -831724440, costs 1103914 ns
for hash result = -831724440, costs 444588 ns (2.5x)
while hash result = -831724440, costs 46510 ns (23x)
```
### Why are the changes needed?
The current WorkerInfo's hashCode() is a little time-consuming. Since it is widely used in lots of hash maps, it needs to be improved.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added UT.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#issuecomment-1805342745
> I found Objects.hash is even several times faster
this is because `Seq` is an implementation of `List`
```scala
// while
startTime = System.nanoTime()
var whileHash = 1
var i = 0
while (i < 5) {
whileHash = 31 * whileHash + state(i).hashCode()
i = i + 1
}
var endTime = System.nanoTime()
println(endTime - startTime)
// while 2
startTime = System.nanoTime()
whileHash = 1
i = 0
while (i < state.length) {
whileHash = 31 * whileHash + state(i).hashCode()
i = i + 1
}
endTime = System.nanoTime()
println(endTime - startTime)
startTime = System.nanoTime()
val objectHash = java.util.Objects.hash(host, rpcPort, pushPort, fetchPort, replicatePort)
endTime = System.nanoTime()
println(endTime - startTime)
```
3924 vs 1821 vs 29459
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#discussion_r1392987698
##########
common/src/main/scala/org/apache/celeborn/common/meta/WorkerInfo.scala:
##########
@@ -232,8 +235,21 @@ class WorkerInfo(
}
override def hashCode(): Int = {
- val state = Seq(host, rpcPort, pushPort, fetchPort, replicatePort)
- state.map(_.hashCode()).foldLeft(0)((a, b) => 31 * a + b)
+ var h = hash
+ if (h == 0 || isZeroHash) {
+ val state = Array(host, rpcPort, pushPort, fetchPort, replicatePort)
+ var i = 0
+ while (i < state.length) {
+ h = 31 * h + state(i).hashCode()
+ i = i + 1
+ }
+ if (h == 0) {
+ isZeroHash = true
+ } else {
+ hash = h
+ }
+ }
+ h
Review Comment:
Couple of points:
* For microbenchmarks, please use JMH - not sure if you did.
* Any implementation which does memoization will typically be faster - since it is if condition + field lookup when result has been memoized - the additional complexity of adding that should be worth it: in this case, how many misses, additional synchronization to ensure visibility of update in a MT safe manner, etc.
From a pure perf pov, you can ofcourse avoid the synchronization overhead by simply pushing the computation to the constructor - then `hashCode` will be simply be returning the computed hash - that will be the fastest :-)
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#issuecomment-1805390352
> btw, we have a benchmark framework introduced in #1657
Get it.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#discussion_r1395465230
##########
common/src/main/scala/org/apache/celeborn/common/meta/WorkerInfo.scala:
##########
@@ -232,8 +232,12 @@ class WorkerInfo(
}
override def hashCode(): Int = {
- val state = Seq(host, rpcPort, pushPort, fetchPort, replicatePort)
- state.map(_.hashCode()).foldLeft(0)((a, b) => 31 * a + b)
+ var result = host.hashCode()
+ result = 31 * result + rpcPort.hashCode()
+ result = 31 * result + pushPort.hashCode()
+ result = 31 * result + fetchPort.hashCode()
+ result = 31 * result + replicatePort.hashCode()
+ result
Review Comment:
Can we cache the hashCode value? seems this method called many times
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#issuecomment-1803377784
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2086?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> :exclamation: No coverage uploaded for pull request base (`main@02cea04`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#section-missing-base-commit).
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## main #2086 +/- ##
=======================================
Coverage ? 46.80%
=======================================
Files ? 166
Lines ? 10703
Branches ? 979
=======================================
Hits ? 5009
Misses ? 5373
Partials ? 321
```
| [Files](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2086?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...a/org/apache/celeborn/common/meta/WorkerInfo.scala](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2086?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL21ldGEvV29ya2VySW5mby5zY2FsYQ==) | `48.86% <100.00%> (ø)` | |
:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in [Chrome](https://chrome.google.com/webstore/detail/codecov/gedikamndpbemklijjkncpnolildpbgo) or [Firefox](https://addons.mozilla.org/en-US/firefox/addon/codecov/) today!
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#issuecomment-1815662280
Thanks for all your help!
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#issuecomment-1805367190
Thanks for @waitinfuture @cfmcgrady suggestions. I will optimize the code and test again.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#issuecomment-1804944666
Per https://github.com/databricks/scala-style-guide#traversal-and-zipwithindex, we need to use `while` loop for performance-sensitive code
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on code in PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#discussion_r1388798668
##########
common/src/test/scala/org/apache/celeborn/common/meta/WorkerInfoSuite.scala:
##########
@@ -303,4 +303,34 @@ class WorkerInfoSuite extends CelebornFunSuite {
}
}
}
+
+ test("Test WorkerInfo hashcode") {
+ val host = "127.0.0.1"
+ val rpcPort = 9082
+ val pushPort = 9092
+ val fetchPort = 9093
+ val replicatePort = 9094
+ val workerInfo = new WorkerInfo(host, rpcPort, pushPort, fetchPort, replicatePort)
+
+ // origin hashCode()
+ var now = System.nanoTime()
+ val state = Seq(host, rpcPort, pushPort, fetchPort, replicatePort)
+ val originHash = state.map(_.hashCode()).foldLeft(0)((a, b) => 31 * a + b)
+ var after = System.nanoTime()
+ println("WorkerInfo origin hash result = " + originHash + ", costs " + (after - now))
Review Comment:
Thanks. I find no need to print logs. Remove done.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#discussion_r1391962579
##########
common/src/main/scala/org/apache/celeborn/common/meta/WorkerInfo.scala:
##########
@@ -232,8 +235,21 @@ class WorkerInfo(
}
override def hashCode(): Int = {
- val state = Seq(host, rpcPort, pushPort, fetchPort, replicatePort)
- state.map(_.hashCode()).foldLeft(0)((a, b) => 31 * a + b)
+ var h = hash
+ if (h == 0 || isZeroHash) {
+ val state = Array(host, rpcPort, pushPort, fetchPort, replicatePort)
+ var i = 0
+ while (i < state.length) {
+ h = 31 * h + state(i).hashCode()
+ i = i + 1
+ }
+ if (h == 0) {
+ isZeroHash = true
+ } else {
+ hash = h
+ }
+ }
+ h
Review Comment:
Instead, why not simply unroll the loop ?
```
override def hashCode: Int = {
var result = if (host != null) host.hashCode else 0
result = 31 * result + rpcPort
result = 31 * result + pushPort
result = 31 * result + fetchPort
result = 31 * result + replicatePort
result
}
```
Since java already caches the hashCode for String, that should be fine.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on code in PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#discussion_r1388798245
##########
common/src/main/scala/org/apache/celeborn/common/meta/WorkerInfo.scala:
##########
@@ -47,6 +47,8 @@ class WorkerInfo(
JavaUtils.newConcurrentHashMap[UserIdentifier, ResourceConsumption](_userResourceConsumption)
else null
var endpoint: RpcEndpointRef = null
+ // Cache the hash code for WorkerInfo
+ var hash = 0
Review Comment:
Done
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li closed pull request #2086: [CELEBORN-1121] Improve WorkerInfo#hashCode method
URL: https://github.com/apache/incubator-celeborn/pull/2086
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#issuecomment-1808235436
> These calculations are sensitive to performance(within milliseconds) and it may be biased when using current benchmark framework.
This is not as expected, can you elaborate more? or show your benchmark code?
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on code in PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#discussion_r1392079035
##########
common/src/main/scala/org/apache/celeborn/common/meta/WorkerInfo.scala:
##########
@@ -232,8 +235,21 @@ class WorkerInfo(
}
override def hashCode(): Int = {
- val state = Seq(host, rpcPort, pushPort, fetchPort, replicatePort)
- state.map(_.hashCode()).foldLeft(0)((a, b) => 31 * a + b)
+ var h = hash
+ if (h == 0 || isZeroHash) {
+ val state = Array(host, rpcPort, pushPort, fetchPort, replicatePort)
+ var i = 0
+ while (i < state.length) {
+ h = 31 * h + state(i).hashCode()
+ i = i + 1
+ }
+ if (h == 0) {
+ isZeroHash = true
+ } else {
+ hash = h
+ }
+ }
+ h
Review Comment:
Thanks @mridulm. I actually thought about that before and have a test shown as below.
```
val host = generateRandomIPv4Address
val rpcPort: Integer = Random.nextInt(65535)
val pushPort: Integer = Random.nextInt(65535)
val fetchPort: Integer = Random.nextInt(65535)
val replicatePort: Integer = Random.nextInt(65535)
val infoSeq = Seq(host, rpcPort, pushPort, fetchPort, replicatePort)
val infoArray = Array(host, rpcPort, pushPort, fetchPort, replicatePort)
// origin
var startTime = System.nanoTime()
infoSeq.map(_.hashCode()).foldLeft(0)((a, b) => 31 * a + b)
var endTime = System.nanoTime()
val originTime = endTime - startTime
println("origin costs %d ns, %.2fx".format(originTime, originTime.toDouble / originTime))
System.gc()
// for seq
startTime = System.nanoTime()
var forHash1 = 0
for (element <- infoSeq) {
forHash1 = 31 * forHash1 + element.hashCode()
}
endTime = System.nanoTime()
val forSeqTime = endTime - startTime
println("for seq costs %d ns, %.2fx".format(forSeqTime, originTime.toDouble / forSeqTime))
System.gc()
// for array
startTime = System.nanoTime()
var forHash2 = 0
for (element <- infoArray) {
forHash2 = 31 * forHash2 + element.hashCode()
}
endTime = System.nanoTime()
val forArrayTime = endTime - startTime
println("for array costs %d ns, %.2fx".format(forArrayTime, originTime.toDouble / forArrayTime))
System.gc()
// while Seq
startTime = System.nanoTime()
var whileHash1 = 0
var i = 0
while (i < infoSeq.size) {
whileHash1 = 31 * whileHash1 + infoSeq(i).hashCode()
i = i + 1
}
endTime = System.nanoTime()
val whileSeqTime = endTime - startTime
println("while seq costs %d ns, %.2fx".format(whileSeqTime, originTime.toDouble / whileSeqTime))
System.gc()
// while array
startTime = System.nanoTime()
var whileHash2 = 0
i = 0
while (i < infoArray.length) {
whileHash2 = 31 * whileHash2 + infoArray(i).hashCode()
i = i + 1
}
endTime = System.nanoTime()
val whileArrayTime = endTime - startTime
println("while array costs %d ns, %.2fx".format(whileArrayTime, originTime.toDouble / whileArrayTime))
System.gc()
// objects hash
startTime = System.nanoTime()
Objects.hash(host, rpcPort, pushPort, fetchPort, replicatePort)
endTime = System.nanoTime()
val objectHashTime = endTime - startTime
println("object hash costs %d ns, %.2fx".format(objectHashTime, originTime.toDouble / objectHashTime))
System.gc()
// direct hash
startTime = System.nanoTime()
var result = host.hashCode()
result = 31 * result + rpcPort.hashCode()
result = 31 * result + pushPort.hashCode()
result = 31 * result + fetchPort.hashCode()
result = 31 * result + replicatePort.hashCode()
endTime = System.nanoTime()
val directHashTime = endTime - startTime
println("directly hash costs %d ns, %.2fx".format(directHashTime, originTime.toDouble / directHashTime))
System.gc()
// direct hash2
startTime = System.nanoTime()
var result2 = 31 * (31 * (31 * (31 * host.hashCode() + rpcPort.hashCode()) + pushPort.hashCode()) + fetchPort.hashCode()) + replicatePort.hashCode()
endTime = System.nanoTime()
val directHashTime2 = endTime - startTime
println("directly hash2 costs %d ns, %.2fx".format(directHashTime2, originTime.toDouble / directHashTime2))
```
And I get the following result:
```
origin costs 1762813 ns, 1.00x
for seq costs 1158450 ns, 1.52x
for array costs 5844185 ns, 0.30x
while seq costs 95599 ns, 18.44x
while array costs 8210 ns, 214.72x
object hash costs 37889 ns, 46.53x
directly hash costs 40092 ns, 43.97x
directly hash2 costs 6498 ns, 271.29x
```
The way mentioned here is called `directly hash`, seems it is worse than that of while array. For the best performance `directly hash2` which expand all calculations, it is a little better than while array. I am also a little confused about the reason here. I think it may be some compiler optimization. And between `directly hash2` and `while array` , I prefer while array because it's more readable.
> we dont need to cache WorkerInfo's hashCode (it is just a few cheap arithmetic computations)
I also think the cache can be removed. The calculation here is really cheap. Introducing the if branch may be worse 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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on code in PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#discussion_r1392094713
##########
common/src/main/scala/org/apache/celeborn/common/meta/WorkerInfo.scala:
##########
@@ -232,8 +235,21 @@ class WorkerInfo(
}
override def hashCode(): Int = {
- val state = Seq(host, rpcPort, pushPort, fetchPort, replicatePort)
- state.map(_.hashCode()).foldLeft(0)((a, b) => 31 * a + b)
+ var h = hash
+ if (h == 0 || isZeroHash) {
+ val state = Array(host, rpcPort, pushPort, fetchPort, replicatePort)
+ var i = 0
+ while (i < state.length) {
+ h = 31 * h + state(i).hashCode()
+ i = i + 1
+ }
+ if (h == 0) {
+ isZeroHash = true
+ } else {
+ hash = h
+ }
+ }
+ h
Review Comment:
I have tested WorkerInfo#hashCode() with and without cache.
```
hashCode() with cache 1 cost 3665 ns
hashCode() with cache 2 cost 472 ns
hashCode() with cache 3 cost 390 ns
hashCode() without cache 1 cost 3389 ns
hashCode() without cache 2 cost 1872 ns
hashCode() without cache 3 cost 1915 ns
```
Since the time unit is small, the improvement is not large in nature, but the cost does not seem to be large. Maybe we can retain the cache？
##########
common/src/main/scala/org/apache/celeborn/common/meta/WorkerInfo.scala:
##########
@@ -232,8 +235,21 @@ class WorkerInfo(
}
override def hashCode(): Int = {
- val state = Seq(host, rpcPort, pushPort, fetchPort, replicatePort)
- state.map(_.hashCode()).foldLeft(0)((a, b) => 31 * a + b)
+ var h = hash
+ if (h == 0 || isZeroHash) {
+ val state = Array(host, rpcPort, pushPort, fetchPort, replicatePort)
+ var i = 0
+ while (i < state.length) {
+ h = 31 * h + state(i).hashCode()
+ i = i + 1
+ }
+ if (h == 0) {
+ isZeroHash = true
+ } else {
+ hash = h
+ }
+ }
+ h
Review Comment:
I have tested WorkerInfo#hashCode() with and without cache.
```
hashCode() with cache 1 cost 3665 ns
hashCode() with cache 2 cost 472 ns
hashCode() with cache 3 cost 390 ns
hashCode() without cache 1 cost 3389 ns
hashCode() without cache 2 cost 1872 ns
hashCode() without cache 3 cost 1915 ns
```
Since the time unit is small, the improvement is not large in nature, but the cost does not seem to be large. Maybe we can retain the cache？
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#issuecomment-1815654043
thanks, @onebox-li @waitinfuture @mridulm @AngersZhuuuu
merging to main(v0.4.0)/branch-0.3(v0.3.2).
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady closed pull request #2086: [CELEBORN-1121] Improve WorkerInfo#hashCode method
URL: https://github.com/apache/incubator-celeborn/pull/2086
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on code in PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#discussion_r1395465323
##########
common/src/main/scala/org/apache/celeborn/common/meta/WorkerInfo.scala:
##########
@@ -232,8 +235,21 @@ class WorkerInfo(
}
override def hashCode(): Int = {
- val state = Seq(host, rpcPort, pushPort, fetchPort, replicatePort)
- state.map(_.hashCode()).foldLeft(0)((a, b) => 31 * a + b)
+ var h = hash
+ if (h == 0 || isZeroHash) {
+ val state = Array(host, rpcPort, pushPort, fetchPort, replicatePort)
+ var i = 0
+ while (i < state.length) {
+ h = 31 * h + state(i).hashCode()
+ i = i + 1
+ }
+ if (h == 0) {
+ isZeroHash = true
+ } else {
+ hash = h
+ }
+ }
+ h
Review Comment:
Thanks @mridulm for suggestion, I did a benchmark, warmed up, and used avg time for comparison.
```
Origin 1.0X
For with Seq 1.1X
For with Array 4.0X
While with Seq 1.6X
While with Array 4.2X
Objects hash 2.1X
Directly hash1 6.9X
Directly hash2 8.1X
No Op 8.5X
```
According to the results, the cost of direct calculation is very cheap. So here remove cache.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#discussion_r1392987698
##########
common/src/main/scala/org/apache/celeborn/common/meta/WorkerInfo.scala:
##########
@@ -232,8 +235,21 @@ class WorkerInfo(
}
override def hashCode(): Int = {
- val state = Seq(host, rpcPort, pushPort, fetchPort, replicatePort)
- state.map(_.hashCode()).foldLeft(0)((a, b) => 31 * a + b)
+ var h = hash
+ if (h == 0 || isZeroHash) {
+ val state = Array(host, rpcPort, pushPort, fetchPort, replicatePort)
+ var i = 0
+ while (i < state.length) {
+ h = 31 * h + state(i).hashCode()
+ i = i + 1
+ }
+ if (h == 0) {
+ isZeroHash = true
+ } else {
+ hash = h
+ }
+ }
+ h
Review Comment:
Couple of points:
* For microbenchmarks, please use JMH - not sure if you did.
* Any implementation which does memoization will typically be faster - since it is if condition + field lookup when result has been memoized - the additional complexity of adding that should be worth it: in this case, synchronization to ensure visibility of update in a MT safe manner.
You can ofcourse avoid the synchronization overhead by simply pushing the computation to the constructor - then `hashCode` will be simply be returning the computed hash - that will be the fastest :-)
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on code in PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#discussion_r1388776573
##########
common/src/main/scala/org/apache/celeborn/common/meta/WorkerInfo.scala:
##########
@@ -47,6 +47,8 @@ class WorkerInfo(
JavaUtils.newConcurrentHashMap[UserIdentifier, ResourceConsumption](_userResourceConsumption)
else null
var endpoint: RpcEndpointRef = null
+ // Cache the hash code for WorkerInfo
+ var hash = 0
Review Comment:
```suggestion
private var hash = 0
```
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#issuecomment-1805300355
I found the following code is even several times faster: 1187677ns vs. 513241ns vs. 32007ns vs. 9436ns
```
val host = generateRandomIPv4Address
val rpcPort: Integer = Random.nextInt(65535)
val pushPort: Integer = Random.nextInt(65535)
val fetchPort: Integer = Random.nextInt(65535)
val replicatePort: Integer = Random.nextInt(65535)
var startTime = System.nanoTime()
val state = Seq(host, rpcPort, pushPort, fetchPort, replicatePort)
// origin
val originHash = state.map(_.hashCode()).foldLeft(1)((a, b) => 31 * a + b)
var endTime = System.nanoTime()
println(endTime - startTime)
// for
startTime = System.nanoTime()
var forHash = 1
for (i <- state) {
forHash = 31 * forHash + i.hashCode()
}
endTime = System.nanoTime()
println(endTime - startTime)
// while
startTime = System.nanoTime()
var whileHash = 1
var i = 0
while (i < state.size) {
whileHash = 31 * whileHash + state(i).hashCode()
i = i + 1
}
endTime = System.nanoTime()
println(endTime - startTime)
startTime = System.nanoTime()
val objectHash = Objects.hash(host, rpcPort, pushPort, fetchPort, replicatePort)
endTime = System.nanoTime()
println(endTime - startTime)
```
But we need to explicitly set type of `rpcPort, pushPort, fetchPort, replicatePort` to `Integer`, or use `Integer.valueOf(rpcPort` int the hash method. In addition, the behavior changes a bit that `Objects.hash` default use 0. IMO it's OK because it only affects the process.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on code in PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#discussion_r1388864241
##########
common/src/test/scala/org/apache/celeborn/common/meta/WorkerInfoSuite.scala:
##########
@@ -303,4 +303,25 @@ class WorkerInfoSuite extends CelebornFunSuite {
}
}
}
+
+ test("Test WorkerInfo hashcode") {
+ val host = "127.0.0.1"
+ val rpcPort = 9082
Review Comment:
Sounds good. Done.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on code in PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#discussion_r1388779635
##########
common/src/test/scala/org/apache/celeborn/common/meta/WorkerInfoSuite.scala:
##########
@@ -303,4 +303,34 @@ class WorkerInfoSuite extends CelebornFunSuite {
}
}
}
+
+ test("Test WorkerInfo hashcode") {
+ val host = "127.0.0.1"
+ val rpcPort = 9082
+ val pushPort = 9092
+ val fetchPort = 9093
+ val replicatePort = 9094
+ val workerInfo = new WorkerInfo(host, rpcPort, pushPort, fetchPort, replicatePort)
+
+ // origin hashCode()
+ var now = System.nanoTime()
+ val state = Seq(host, rpcPort, pushPort, fetchPort, replicatePort)
+ val originHash = state.map(_.hashCode()).foldLeft(0)((a, b) => 31 * a + b)
+ var after = System.nanoTime()
+ println("WorkerInfo origin hash result = " + originHash + ", costs " + (after - now))
Review Comment:
avoid call `println` in UTs
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on code in PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#discussion_r1388815651
##########
common/src/test/scala/org/apache/celeborn/common/meta/WorkerInfoSuite.scala:
##########
@@ -303,4 +303,25 @@ class WorkerInfoSuite extends CelebornFunSuite {
}
}
}
+
+ test("Test WorkerInfo hashcode") {
+ val host = "127.0.0.1"
+ val rpcPort = 9082
Review Comment:
I think we can randomize this unit test
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#discussion_r1391962579
##########
common/src/main/scala/org/apache/celeborn/common/meta/WorkerInfo.scala:
##########
@@ -232,8 +235,21 @@ class WorkerInfo(
}
override def hashCode(): Int = {
- val state = Seq(host, rpcPort, pushPort, fetchPort, replicatePort)
- state.map(_.hashCode()).foldLeft(0)((a, b) => 31 * a + b)
+ var h = hash
+ if (h == 0 || isZeroHash) {
+ val state = Array(host, rpcPort, pushPort, fetchPort, replicatePort)
+ var i = 0
+ while (i < state.length) {
+ h = 31 * h + state(i).hashCode()
+ i = i + 1
+ }
+ if (h == 0) {
+ isZeroHash = true
+ } else {
+ hash = h
+ }
+ }
+ h
Review Comment:
Instead, why not simply unroll the loop ?
```
override def hashCode: Int = {
var result = if (host != null) host.hashCode else 0
result = 31 * result + rpcPort
result = 31 * result + pushPort
result = 31 * result + fetchPort
result = 31 * result + replicatePort
result
}
```
Since java already caches the hashCode for String, that should be fine and we dont need to cache WorkerInfo's hashCode (it is just a few cheap arithmetic computations)
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#issuecomment-1807862778
These calculations are sensitive to performance(within milliseconds) and it may be biased when using current benchmark framework. So I tested them locally and the results are more intuitive, shown below.
```
java version "1.8.0_261"
=====================
origin costs 1026147 ns, 1.00x
for seq costs 791501 ns, 1.30x
for array costs 336341 ns, 3.05x
while seq costs 36436 ns, 28.16x
while array costs 1082 ns, 948.38x
object hash costs 9595 ns, 106.95x
```
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on code in PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#discussion_r1388782549
##########
common/src/test/scala/org/apache/celeborn/common/meta/WorkerInfoSuite.scala:
##########
@@ -303,4 +303,34 @@ class WorkerInfoSuite extends CelebornFunSuite {
}
}
}
+
+ test("Test WorkerInfo hashcode") {
+ val host = "127.0.0.1"
+ val rpcPort = 9082
+ val pushPort = 9092
+ val fetchPort = 9093
+ val replicatePort = 9094
+ val workerInfo = new WorkerInfo(host, rpcPort, pushPort, fetchPort, replicatePort)
+
+ // origin hashCode()
+ var now = System.nanoTime()
+ val state = Seq(host, rpcPort, pushPort, fetchPort, replicatePort)
+ val originHash = state.map(_.hashCode()).foldLeft(0)((a, b) => 31 * a + b)
+ var after = System.nanoTime()
+ println("WorkerInfo origin hash result = " + originHash + ", costs " + (after - now))
Review Comment:
use `log` or `assert(condition: Boolean, clue: Any)` instead of `println`.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#issuecomment-1805377388
btw, we have a benchmark framework introduced in https://github.com/apache/incubator-celeborn/pull/1657
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on code in PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#discussion_r1391162601
##########
common/src/main/scala/org/apache/celeborn/common/meta/WorkerInfo.scala:
##########
@@ -232,8 +235,21 @@ class WorkerInfo(
}
override def hashCode(): Int = {
- val state = Seq(host, rpcPort, pushPort, fetchPort, replicatePort)
- state.map(_.hashCode()).foldLeft(0)((a, b) => 31 * a + b)
+ var h = hash
+ if (h == 0 || isZeroHash) {
+ val state = Array(host, rpcPort, pushPort, fetchPort, replicatePort)
+ var i = 0
+ while (i < state.length) {
+ h = 31 * h + state(i).hashCode()
+ i = i + 1
+ }
+ if (h == 0) {
+ isZeroHash = true
+ } else {
+ hash = h
Review Comment:
it's not thread-safe, shall we need to cache the hash code?
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#discussion_r1391962579
##########
common/src/main/scala/org/apache/celeborn/common/meta/WorkerInfo.scala:
##########
@@ -232,8 +235,21 @@ class WorkerInfo(
}
override def hashCode(): Int = {
- val state = Seq(host, rpcPort, pushPort, fetchPort, replicatePort)
- state.map(_.hashCode()).foldLeft(0)((a, b) => 31 * a + b)
+ var h = hash
+ if (h == 0 || isZeroHash) {
+ val state = Array(host, rpcPort, pushPort, fetchPort, replicatePort)
+ var i = 0
+ while (i < state.length) {
+ h = 31 * h + state(i).hashCode()
+ i = i + 1
+ }
+ if (h == 0) {
+ isZeroHash = true
+ } else {
+ hash = h
+ }
+ }
+ h
Review Comment:
Instead, why not simply unroll the loop ?
```
override def hashCode: Int = {
var result = if (host != null) host.hashCode else 0
result = 31 * result + rpcPort
result = 31 * result + pushPort
result = 31 * result + fetchPort
result = 31 * result + replicatePort
result
}
```
Since java already caches the hashCode for String, that should be fine and we dont need to cache WorkerInfo's hashCode (it is just a few cheap arthithematic computations)
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1121] Improve WorkerInfo#hashCode method [incubator-celeborn]
Posted by "onebox-li (via GitHub)" <gi...@apache.org>.
onebox-li commented on code in PR #2086:
URL: https://github.com/apache/incubator-celeborn/pull/2086#discussion_r1395497213
##########
common/src/main/scala/org/apache/celeborn/common/meta/WorkerInfo.scala:
##########
@@ -232,8 +232,12 @@ class WorkerInfo(
}
override def hashCode(): Int = {
- val state = Seq(host, rpcPort, pushPort, fetchPort, replicatePort)
- state.map(_.hashCode()).foldLeft(0)((a, b) => 31 * a + b)
+ var result = host.hashCode()
+ result = 31 * result + rpcPort.hashCode()
+ result = 31 * result + pushPort.hashCode()
+ result = 31 * result + fetchPort.hashCode()
+ result = 31 * result + replicatePort.hashCode()
+ result
Review Comment:
The calculation here is relatively cheap, and the cache will not be much improved, so we may make it simpler.
--
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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org