Skip to content

[SPARK-5282][mllib]: RowMatrix easily gets int overflow in the memory size warning #4069

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[SPARK-5282][mllib]: RowMatrix easily gets int overflow in the memory size warning #4069

wants to merge 2 commits into from

Conversation

@hhbyyh
Copy link
Contributor

@hhbyyh hhbyyh commented Jan 16, 2015

JIRA: https://issues.apache.org/jira/browse/SPARK-5282

fix the possible int overflow in the memory computation warning

@hhbyyh hhbyyh changed the title [SPARK-5282][mllib]: fix int overflow in the warning [SPARK-5282][mllib]: RowMatrix easily gets int overflow in the memory size warning Jan 16, 2015
@SparkQA
Copy link

SparkQA commented Jan 16, 2015

Test build #25657 has started for PR 4069 at commit 7afac23.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 16, 2015

Test build #25657 has finished for PR 4069 at commit 7afac23.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

AmplabJenkins commented Jan 16, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25657/
Test PASSed.

srowen
srowen reviewed Jan 16, 2015
View reviewed changes
mllib/src/main/scala/org/apache/spark/mllib/linalg/distributed/RowMatrix.scala Outdated
Copy link
Member

@srowen srowen Jan 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's minor, but I think it might be simpler to report megabytes in the warning message, since the reported size is at least 800MB. Then this can be simply val memMB = (cols.toLong * cols) / 125000; that multiplication can't overflow.

Copy link
Contributor Author

@hhbyyh hhbyyh Jan 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely I can do it. I was worrying some one may ask for the accurate number in the PR~~ :)

Another thing is, /125000 might should be changed to >> 15?

Copy link
Member

@srowen srowen Jan 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

>> 17 ? a mebibyte is 2^20 bytes so that would make sense. I don't know what the overall stance on mega vs mebibyte is in the nomenclature here but think everything is megabytes, in which case / 125000 is right.

Copy link
Contributor Author

@hhbyyh hhbyyh Jan 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, 17...I'll send update tomorrow morning.

Copy link
Contributor Author

@hhbyyh hhbyyh Jan 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this 1000/1024 thing is why they send actual bytes in the beginning... :)

@SparkQA
Copy link

SparkQA commented Jan 17, 2015

Test build #25700 has started for PR 4069 at commit e54e5c8.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 17, 2015

Test build #25700 has finished for PR 4069 at commit e54e5c8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

AmplabJenkins commented Jan 17, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25700/
Test PASSed.

@hhbyyh
Copy link
Contributor Author

hhbyyh commented Jan 19, 2015

@srowen Would you mind to take another look? Thanks

@srowen
Copy link
Member

srowen commented Jan 19, 2015

LGTM

@hhbyyh
Copy link
Contributor Author

hhbyyh commented Jan 19, 2015

Thanks.

asfgit pushed a commit that referenced this pull request Jan 19, 2015
@hhbyyh @mengxr
[SPARK-5282][mllib]: RowMatrix easily gets int overflow in the memory…
5d5ee40 
… size warning

JIRA: https://issues.apache.org/jira/browse/SPARK-5282

fix the possible int overflow in the memory computation warning

Author: Yuhao Yang <[email protected]>

Closes #4069 from hhbyyh/addscStop and squashes the following commits:

e54e5c8 [Yuhao Yang] change to MB based number
7afac23 [Yuhao Yang] 5282: fix int overflow in the warning

(cherry picked from commit 4432568)
Signed-off-by: Xiangrui Meng <[email protected]>
@mengxr
Copy link
Contributor

mengxr commented Jan 19, 2015

Merged into master and branch-1.2. Thanks!

@asfgit asfgit closed this in 4432568 Jan 19, 2015
@mengxr mengxr mentioned this pull request Jan 27, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@hhbyyh @SparkQA @AmplabJenkins @srowen @mengxr