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

[Core] dlmalloc allocate bottom-most memory chunk failed #21439

Merged
merged 6 commits into from
Jan 14, 2022

Conversation

Catch-Bull
Copy link
Contributor

@Catch-Bull Catch-Bull commented Jan 7, 2022

Why are these changes needed?

fix dlmalloc allocate bug, details in here #21310

Related issue number

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@raulchen raulchen requested review from ericl and scv119 January 7, 2022 03:45
@ericl ericl self-assigned this Jan 7, 2022
@jjyao
Copy link
Collaborator

jjyao commented Jan 7, 2022

@Catch-Bull The issue you linked is a Ray Tune issue, is it the correct one?

# keep reference for fourth object, avoid released by plasma GC.
__ = ray.put(data) # noqa

# Check fourth object whether allocate to disk
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use check_spilled_mb to see if it was disk allocated instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@@ -56,6 +56,9 @@ int fake_munmap(void *, int64_t);
#define HAVE_MORECORE 0
#define DEFAULT_MMAP_THRESHOLD MAX_SIZE_T
#define DEFAULT_GRANULARITY ((size_t)128U * 1024U)
// Copied from plasma_allocator.cc variable kAllocationAlignment,
// make sure to keep in sync
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a link to GH issue for context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 7, 2022
@Catch-Bull
Copy link
Contributor Author

@Catch-Bull The issue you linked is a Ray Tune issue, is it the correct one?

oh... it's my mistake, I've made it correct.

__ = ray.put(data) # noqa

# Check fourth object allocate in memory.
_check_spilled_mb(address, spilled=180)
Copy link
Contributor

Choose a reason for hiding this comment

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

you can also write a unit test in C++ demonstrate the issue. Here are some examples you can follow:
https://github.com/ray-project/ray/blob/master/src/ray/object_manager/plasma/test/fallback_allocator_test.cc#L35

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@scv119 scv119 linked an issue Jan 9, 2022 that may be closed by this pull request
2 tasks
@scv119
Copy link
Contributor

scv119 commented Jan 11, 2022

Thanks for addressing the comments. Looks the lint issues is unrelated but let me retry them before merging.

@scv119 scv119 added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Jan 14, 2022
@scv119 scv119 merged commit ded4128 into ray-project:master Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] dlmalloc allocate bottom-most memory chunk failed
4 participants