Skip to content

[WIP][Core] Optional Profiling. #9614

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

Closed
rkooo567 wants to merge 2 commits into from
Closed

[WIP][Core] Optional Profiling. #9614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a43ce00
Initial progress.
rkooo567 Jul 21, 2020
1e9caf4
Merge branch 'master' into optional-profiling
rkooo567 Jul 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions python/ray/_raylet.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1009,6 +1009,10 @@ cdef class CoreWorker:
return resources_dict

def profile_event(self, c_string event_type, object extra_data=None):
# Turn off profiling by default.
if not RayConfig.instance().run_profiling():
return NoopProfileEvent()

return ProfileEvent.make(
CCoreWorkerProcess.GetCoreWorker().CreateProfileEvent(event_type),
extra_data)
Expand Down
9 changes: 9 additions & 0 deletions python/ray/includes/libcoreworker.pxi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,12 @@ cdef class ProfileEvent:
# Deleting the CProfileEvent will add it to a queue to be pushed to
# the driver.
self.inner.reset()


cdef class NoopProfileEvent:
"""Noop profile event used when profiling is turned off."""
def __enter__(self):
pass

def __exit__(self, type, value, tb):
pass
2 changes: 2 additions & 0 deletions python/ray/includes/ray_config.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,3 +92,5 @@ cdef extern from "ray/common/ray_config.h" nogil:
c_bool put_small_object_in_memory_store() const

uint32_t max_tasks_in_flight_per_worker() const

c_bool run_profiling() const
4 changes: 4 additions & 0 deletions src/ray/common/ray_config_def.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,3 +329,7 @@ RAY_CONFIG(bool, put_small_object_in_memory_store, false)
/// the owner has been granted a lease. A value >1 is used when we want to enable
/// pipelining task submission.
RAY_CONFIG(uint32_t, max_tasks_in_flight_per_worker, 1)

RAY_CONFIG(bool, run_profiling,
getenv("RAY_PROFILE_ENABLED") != nullptr &&
getenv("RAY_PROFILE_ENABLED") == std::string("true"))
Comment on lines +333 to +335
Copy link
Copy Markdown
Contributor

@simon-mo simon-mo Jul 21, 2020

Choose a reason for hiding this comment

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

We probably shouldn't feature flag it via environment variable? Seems to me just enable it via _internal_config is better.

Copy link
Copy Markdown
Contributor Author

@rkooo567 rkooo567 Jul 22, 2020

Choose a reason for hiding this comment

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

One concern I have is that _internal_config (not sure if it is still the case) is not meant to be exposed to users, but this feature is supposed to be exposed to users.