Skip to content

[core] split plasma_store_server_lib into smaller targets#51825

Merged
jjyao merged 2 commits intoray-project:masterfrom
Ziy1-Tan:bazel_plasma_store
Apr 2, 2025
Merged

[core] split plasma_store_server_lib into smaller targets#51825
jjyao merged 2 commits intoray-project:masterfrom
Ziy1-Tan:bazel_plasma_store

Conversation

@Ziy1-Tan
Copy link
Copy Markdown
Contributor

@Ziy1-Tan Ziy1-Tan commented Mar 30, 2025 •
edited
Loading

Why are these changes needed?

Related issue number

Closes #51823

bazel build //:plasma_store_server_lib:

Target //:plasma_store_server_lib up-to-date:
  bazel-bin/libplasma_store_server_lib.a
  bazel-bin/libplasma_store_server_lib.pic.a
  bazel-bin/libplasma_store_server_lib.so
INFO: Elapsed time: 383.045s, Critical Path: 60.86s
INFO: 3872 processes: 1609 internal, 2263 linux-sandbox.
INFO: Build completed successfully, 3872 total actions



Target //:plasma_store_server_lib up-to-date:
  bazel-bin/libplasma_store_server_lib.a
  bazel-bin/libplasma_store_server_lib.pic.a
  bazel-bin/libplasma_store_server_lib.so
INFO: Elapsed time: 186.160s, Critical Path: 42.49s
INFO: 2250 processes: 1506 internal, 744 linux-sandbox.
INFO: Build completed successfully, 2250 total actions

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • 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 added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • 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 :(

@Ziy1-Tan Ziy1-Tan force-pushed the bazel_plasma_store branch 2 times, most recently from a201fb4 to c682185 Compare March 31, 2025 06:25
@jcotant1 jcotant1 added the core Issues that should be addressed in Ray Core label Mar 31, 2025
@Ziy1-Tan Ziy1-Tan force-pushed the bazel_plasma_store branch 4 times, most recently from d60f89b to 1d5f0e0 Compare April 2, 2025 04:50
@Ziy1-Tan
Copy link
Copy Markdown
Contributor Author

Ziy1-Tan commented Apr 2, 2025

cc @dentiny .

@dentiny dentiny added the go add ONLY when ready to merge, run all tests label Apr 2, 2025
@dentiny
Copy link
Copy Markdown
Contributor

dentiny commented Apr 2, 2025

Just enabled CI for you, ping me when it's ready :)

dentiny
dentiny reviewed Apr 2, 2025
View reviewed changes
BUILD.bazel Outdated
Copy link
Copy Markdown
Contributor

@dentiny dentiny Apr 2, 2025

Choose a reason for hiding this comment

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

please prefix with plasma, malloc is too general (this is a root build file)

dentiny
dentiny reviewed Apr 2, 2025
View reviewed changes
BUILD.bazel Outdated
Copy link
Copy Markdown
Contributor

@dentiny dentiny Apr 2, 2025

Choose a reason for hiding this comment

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

please prefix with plasma

dentiny
dentiny reviewed Apr 2, 2025
View reviewed changes
src/ray/object_manager/plasma/eviction_policy.h Outdated
Copy link
Copy Markdown
Contributor

@dentiny dentiny Apr 2, 2025

Choose a reason for hiding this comment

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

Thanks for the cleanup!

dentiny
dentiny reviewed Apr 2, 2025
View reviewed changes
BUILD.bazel Outdated
Copy link
Copy Markdown
Contributor

@dentiny dentiny Apr 2, 2025

Choose a reason for hiding this comment

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

Should we use gtest_prod?

dentiny
dentiny reviewed Apr 2, 2025
View reviewed changes
BUILD.bazel Outdated
Copy link
Copy Markdown
Contributor

@dentiny dentiny Apr 2, 2025

Choose a reason for hiding this comment

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

btw, I quickly checked manually, unordered_map and unordered_set is not used anywhere, could you please do me a favor to remove them from inclusion?

Copy link
Copy Markdown
Contributor

@dentiny dentiny Apr 2, 2025

Choose a reason for hiding this comment

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

meanwhile, would be better to include C++ headers instead of C ones.

ray/src/ray/object_manager/plasma/store.cc

Lines 31 to 33 in aa2dede

#include <limits.h>
#include <stdlib.h>
#include <string.h>

In detail, limits,h -> climits

@Ziy1-Tan Ziy1-Tan force-pushed the bazel_plasma_store branch from 1d5f0e0 to 02dc191 Compare April 2, 2025 14:20
Ziy1-Tan added 2 commits April 2, 2025 23:43
@Ziy1-Tan
[core] split plasma_store_server_lib into smaller targets to improve …
2d3a74d 
…build performance

Signed-off-by: Ziy1-Tan <ajb459684460@gmail.com>
@Ziy1-Tan
Fix code style
0850a33 
Signed-off-by: Ziy1-Tan <ajb459684460@gmail.com>
@Ziy1-Tan Ziy1-Tan force-pushed the bazel_plasma_store branch from 02dc191 to 0850a33 Compare April 2, 2025 16:02
@Ziy1-Tan Ziy1-Tan requested a review from dentiny April 2, 2025 23:35
dentiny
dentiny approved these changes Apr 2, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@dentiny dentiny left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@dentiny
Copy link
Copy Markdown
Contributor

dentiny commented Apr 2, 2025

@aslonnie / @edoakes / @jjyao could you please help merge the PR? Thanks!

@jjyao jjyao merged commit 52899a8 into ray-project:master Apr 2, 2025
5 checks passed
@dentiny dentiny mentioned this pull request Apr 3, 2025
Merged
@hainesmichaelc hainesmichaelc added the community-contribution Contributed by the community label Apr 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dentiny dentiny dentiny approved these changes

Assignees

No one assigned

Labels

community-backlog community-contribution Contributed by the community core Issues that should be addressed in Ray Core go add ONLY when ready to merge, run all tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[core/plasma_store] Split giant ray core C++ target into small ones

5 participants

@Ziy1-Tan @dentiny @jjyao @hainesmichaelc @jcotant1