Skip to content
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

enable multithreading tests of joins only on 64 bit machines #3183

Merged
merged 2 commits into from
Sep 27, 2022
Merged

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Sep 26, 2022

@nalimilan - the multithreading tests for 32-bit machine constantly fail. I think it is better to just disable them in that case.

@bkamins bkamins added the CI label Sep 26, 2022
@bkamins bkamins added this to the 1.4 milestone Sep 26, 2022
@nalimilan
Copy link
Member

Isn't the current try... catch OutOfMemoryError enough?

@bkamins bkamins closed this Sep 26, 2022
@bkamins bkamins reopened this Sep 26, 2022
@bkamins
Copy link
Member Author

bkamins commented Sep 26, 2022

First CI run passed cleanly. I closed and reopened it to make sure it is stable.

@bkamins
Copy link
Member Author

bkamins commented Sep 26, 2022

Isn't the current try... catch OutOfMemoryError enough?

No because Julia does not handle correctly such try-catch and passes OOM to LLVM which crashes Julia. I discussed it with @vtjnash and @gbaraldi on Slack and it seems it is not fixable.

In short (from Slack):

by catching the exception, you have likely ensured that there is no available memory to reclaim

i.e. because we do try-catch Julia cannot do GC and crashes 😞.

test/join.jl Outdated Show resolved Hide resolved
test/join.jl Outdated Show resolved Hide resolved
@bkamins bkamins merged commit 1615476 into main Sep 27, 2022
@bkamins bkamins deleted the bk/fix_OOM branch September 27, 2022 12:38
@bkamins
Copy link
Member Author

bkamins commented Sep 27, 2022

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants