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

[Shuffle] Skip store shuffle object refs to reduce meta overhead #3209

Conversation

chaokunyang
Copy link
Contributor

@chaokunyang chaokunyang commented Aug 8, 2022

What do these changes do?

This PR skip store shuffle object refs to reduce meta overhead and supervisor serialization bottleneck by disable autoscale-in when shuffle is executing.

Related issue number

#2916

Check code requirements

  • tests added / passed (if needed)
  • Ensure all linting tests pass, see here for how to run them

@chaokunyang chaokunyang requested a review from a team as a code owner August 8, 2022 11:47
@chaokunyang chaokunyang marked this pull request as draft August 8, 2022 11:48
@chaokunyang chaokunyang force-pushed the skip_store_shuffle_object_refs_when_autoscale_disabled branch 2 times, most recently from 1f99c3d to a037db0 Compare August 9, 2022 04:57
@chaokunyang chaokunyang marked this pull request as ready for review August 9, 2022 06:56
@chaokunyang chaokunyang changed the title [Shuffle] Skip store shuffle object refs when autoscale disabled [Shuffle] Skip store shuffle object refs to reduce meta overhead Aug 9, 2022
zhongchun
zhongchun previously approved these changes Aug 12, 2022
Copy link
Contributor

@zhongchun zhongchun left a comment

Choose a reason for hiding this comment

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

LGTM.

cluster_api = await ClusterAPI.create(address)
supervisor_address = (await cluster_api.get_supervisors())[0]
autoscaler = await mo.actor_ref(
AutoscalerActor.default_uid(), address=supervisor_address
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use address=address? The address is the current supervisor address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If supervisor created subpools and current process is subpool, then address won't be supervisor address

Copy link
Contributor

Choose a reason for hiding this comment

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

But, the SubtaskQueueingActor, SubtaskManagerActor and AutoscalerActor are all created in the same address. So, the actor refs should be created from the same address, too. Please refer to: https://github.com/mars-project/mars/blob/master/mars/services/scheduling/supervisor/service.py#L122

Can you unify the address of actor ref creations of SubtaskQueueingActor, SubtaskManagerActor and AutoscalerActor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SubtaskQueueingActor and SubtaskManagerActor may be created in the subpool. The AutoscalerActor will always be created in the supervisor main pool, it's similar to GlobaleResourceManager.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AutoscalerActor

It's different address, the address ofSubtaskQueueingActor/SubtaskManagerActor/AutoscalerActor may be subpool address, AutoscalerActor will always use main pool address.

I changed actror_ref creation of AutoscalerActor to:

cluster_api = await ClusterAPI.create(address)
[autoscaler] = await cluster_api.get_supervisor_refs(
    [AutoscalerActor.default_uid()]
)

mars/services/subtask/core.py Outdated Show resolved Hide resolved
mars/services/subtask/core.py Outdated Show resolved Hide resolved
if self._has_shuffle:
mapper_chunks, proxy_chunks = [], []
for c in result_chunks:
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is only for the ShuffleFetchType.FETCH_BY_INDEX? Record the shuffle proxy subtask reduces duplicate search shuffle proxy overhead, but introduces overhead for ShuffleFetchType.FETCH_BY_KEY.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used by TaskStageProcessor

mars/services/task/analyzer/analyzer.py Outdated Show resolved Hide resolved
@chaokunyang chaokunyang force-pushed the skip_store_shuffle_object_refs_when_autoscale_disabled branch from 05c8b0b to 6170d0b Compare August 12, 2022 09:04
@chaokunyang chaokunyang force-pushed the skip_store_shuffle_object_refs_when_autoscale_disabled branch from 6170d0b to 4d4a4b4 Compare September 5, 2022 06:59
@chaokunyang chaokunyang force-pushed the skip_store_shuffle_object_refs_when_autoscale_disabled branch from 4d4a4b4 to 3a93dc1 Compare September 5, 2022 08:06
Copy link
Contributor

@fyrestone fyrestone left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@zhongchun zhongchun left a comment

Choose a reason for hiding this comment

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

LGTM.

@chaokunyang chaokunyang merged commit b2d658e into mars-project:master Sep 6, 2022
aresnow1 pushed a commit to aresnow1/mars that referenced this pull request Sep 7, 2022
…s-project#3209)

* disable shuffle in autoscale to skip shuffle meta

* fix remove mapper data

* refine autoscale in

* fix SubtaskGraph add proxy chunks

* add shuffle tests to autoscale

* fxi mapper chunks check

* remove unnecessary event

* refine proxy_subtasks check

* workaround versionner compatibility with PEP660

* fix get autoscaler

* fix subtask graph building

(cherry picked from commit b2d658e)
qianduoduo0904 pushed a commit to qianduoduo0904/mars that referenced this pull request Sep 22, 2022
…s-project#3209)

* disable shuffle in autoscale to skip shuffle meta

* fix remove mapper data

* refine autoscale in

* fix SubtaskGraph add proxy chunks

* add shuffle tests to autoscale

* fxi mapper chunks check

* remove unnecessary event

* refine proxy_subtasks check

* workaround versionner compatibility with PEP660

* fix get autoscaler

* fix subtask graph building

(cherry picked from commit b2d658e)
UranusSeven pushed a commit to xorbitsai/mars that referenced this pull request Sep 22, 2022
…s-project#3209)

* disable shuffle in autoscale to skip shuffle meta

* fix remove mapper data

* refine autoscale in

* fix SubtaskGraph add proxy chunks

* add shuffle tests to autoscale

* fxi mapper chunks check

* remove unnecessary event

* refine proxy_subtasks check

* workaround versionner compatibility with PEP660

* fix get autoscaler

* fix subtask graph building

(cherry picked from commit b2d658e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants