[SPARK-20587][ML] Improve performance of ML ALS recommendForAll #17845
Conversation
|
cc @mpjlu Also @srowen @sethah @jkbradley |
|
Some quick perf numbers: Using
So 23-37x improvement. |
|
Test build #76424 has finished for PR 17845 at commit
|
| score += srcFactor(k) * dstFactor(k) | ||
| k += 1 | ||
| } | ||
| pq += { (dstId, score) } |
| @@ -389,6 +436,17 @@ class ALSModel private[ml] ( | |||
| ) | |||
| recs.select($"id" as srcOutputColumn, $"recommendations" cast arrayType) | |||
There was a problem hiding this comment.
This is discouraged within Spark: https://github.com/databricks/scala-style-guide#infix-methods
There was a problem hiding this comment.
Fair point - may as well fix it while here
| */ | ||
| private def blockify( | ||
| factors: Dataset[(Int, Array[Float])], | ||
| /* TODO make blockSize a param? */blockSize: Int = 4096): Dataset[Seq[(Int, Array[Float])]] = { |
There was a problem hiding this comment.
just put the comment in the doc and reference a JIRA.
| * relatively efficient, the approach implemented here is significantly more efficient. | ||
| * | ||
| * This approach groups factors into blocks and computes the top-k elements per block, | ||
| * using Level 1 BLAS (dot) and an efficient [[BoundedPriorityQueue]]. It then computes the |
There was a problem hiding this comment.
below we say that blas is not used.
There was a problem hiding this comment.
How about "... using dot product instead of gemm and an efficient ..."
| val pq = new BoundedPriorityQueue[(Int, Float)](num)(Ordering.by(_._2)) | ||
| srcIter.foreach { case (srcId, srcFactor) => | ||
| dstIter.foreach { case (dstId, dstFactor) => | ||
| /** |
There was a problem hiding this comment.
don't use doc notation. Maybe we can reduce it to:
/*
* The below code is equivalent to
* `val score = blas.sdot(rank, srcFactor, 1, dstFactor, 1)`
* The handwritten version is as or more efficient as BLAS calls in this case.
*/
|
Thanks @sethah will update shortly |
|
Test build #76447 has finished for PR 17845 at commit
|
|
jenkins retest this please |
|
Test build #76571 has finished for PR 17845 at commit
|
| val m = srcIter.size | ||
| val n = math.min(dstIter.size, num) | ||
| val output = new Array[(Int, Int, Float)](m * n) | ||
| var j = 0 |
There was a problem hiding this comment.
Nit: You could combine j and i; you really just need 1 counter.
There was a problem hiding this comment.
j iterates through src ids while i iterates through dst ids in the queue for each src id. So I don't think they can be combined.
There was a problem hiding this comment.
Anyway the iter.next() code is a bit ugly and since it's at most k elements it's not really performance critical, so could just use foreach I think
|
One more comment I'll copy from the other PR: I'm not a fan of custom BLAS implementations scattered throughout MLlib. Could you please follow up by putting the dot as a private API in BLAS.scala and adding unit tests? |
|
Merged to master/branch-2.2 Thanks @mpjlu for the original work on the approach! |
This PR is a `DataFrame` version of #17742 for [SPARK-11968](https://issues.apache.org/jira/browse/SPARK-11968), for improving the performance of `recommendAll` methods. ## How was this patch tested? Existing unit tests. Author: Nick Pentreath <[email protected]> Closes #17845 from MLnick/ml-als-perf. (cherry picked from commit 10b00ab) Signed-off-by: Nick Pentreath <[email protected]>
Small clean ups from #17742 and #17845. ## How was this patch tested? Existing unit tests. Author: Nick Pentreath <[email protected]> Closes #17919 from MLnick/SPARK-20677-als-perf-followup. (cherry picked from commit 25b4f41) Signed-off-by: Nick Pentreath <[email protected]>
Small clean ups from apache#17742 and apache#17845. ## How was this patch tested? Existing unit tests. Author: Nick Pentreath <[email protected]> Closes apache#17919 from MLnick/SPARK-20677-als-perf-followup.
Small clean ups from apache#17742 and apache#17845. ## How was this patch tested? Existing unit tests. Author: Nick Pentreath <[email protected]> Closes apache#17919 from MLnick/SPARK-20677-als-perf-followup.
This PR is a `DataFrame` version of apache#17742 for [SPARK-11968](https://issues.apache.org/jira/browse/SPARK-11968), for improving the performance of `recommendAll` methods. ## How was this patch tested? Existing unit tests. Author: Nick Pentreath <[email protected]> Closes apache#17845 from MLnick/ml-als-perf.
Small clean ups from apache#17742 and apache#17845. ## How was this patch tested? Existing unit tests. Author: Nick Pentreath <[email protected]> Closes apache#17919 from MLnick/SPARK-20677-als-perf-followup.
This PR is a
DataFrameversion of #17742 for SPARK-11968, for improving the performance ofrecommendAllmethods.How was this patch tested?
Existing unit tests.