-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-20443][MLLIB][ML] set ALS blockify size #17739
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Test build #76096 has started for PR 17739 at commit |
|
Just to confirm, the #users is 48 million, #items is 1.7 million? |
|
Or is it 48,000 and 1,700? |
|
users is 480,000, items is 17,000. Thanks |
|
ok. And it is the timing for |
|
RecommandProductsForUsers. Thanks |
|
It's interesting to see the performance difference. I've also been looking at performance of recommend all but haven't gotten to varying the block sizes just yet. I'm potentially in favor of exposing it as a param - but what you've got here doesn't do anything to the public API so how does that help? |
|
Test build #76108 has finished for PR 17739 at commit
|
|
Test build #76775 has finished for PR 17739 at commit
|
|
Test build #76781 has started for PR 17739 at commit |
|
Test build #76787 has finished for PR 17739 at commit
|
|
Test build #76971 has finished for PR 17739 at commit
|
|
retest this please |
|
Test build #76998 has finished for PR 17739 at commit
|
|
Because I don't have the environment to continue this work, I will close it. Thanks. |
What changes were proposed in this pull request?
The blockSize of MLLIB ALS is very important for ALS performance.
In our test, when the blockSize is 128, the performance is about 4X comparing with the blockSize is 4096 (default value).
The following are our test results:
BlockSize(recommendationForAll time)
128(124s), 256(160s), 512(184s), 1024(244s), 2048(332s), 4096(488s), 8192(OOM)
The Test Environment:
3 workers: each work 10 core, each work 30G memory, each work 1 executor.
The Data: User 480,000, and Item 17,000
How was this patch tested?
The existing UT