You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@texera.apache.org by "Xiao-zhen-Liu (via GitHub)" <gi...@apache.org> on 2025/08/14 01:16:37 UTC
[PR] feat(amber): add memoization of region costs in CostBasedScheduleGenerator [texera]
Xiao-zhen-Liu opened a new pull request, #3660:
URL: https://github.com/apache/texera/pull/3660
As `CostEstimator.allocateResourcesAndEstimateCost` can be expensive, this PR adds memoization of region costs (and also updated region with resource configs) during the search process of `CostBasedScheduleGenerator`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(amber): add memoization of region costs in CostBasedScheduleGenerator [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on code in PR #3660:
URL: https://github.com/apache/texera/pull/3660#discussion_r2299588834
##########
core/amber/src/main/scala/edu/uci/ics/amber/engine/architecture/scheduling/CostBasedScheduleGenerator.scala:
##########
@@ -606,8 +610,8 @@ class CostBasedScheduleGenerator(
.map(level =>
level
.map(region => {
- val (resourceConfig, regionCost) =
- costEstimator.allocateResourcesAndEstimateCost(region, 1)
+ val (resourceConfig, regionCost) = costEstimatorMemoization
+ .getOrElseUpdate(region, costEstimator.allocateResourcesAndEstimateCost(region, 1))
Review Comment:
Do we really need the entire region as the key? That's a lot of information.
##########
core/amber/src/main/scala/edu/uci/ics/amber/engine/architecture/scheduling/CostBasedScheduleGenerator.scala:
##########
@@ -606,8 +610,8 @@ class CostBasedScheduleGenerator(
.map(level =>
level
.map(region => {
- val (resourceConfig, regionCost) =
- costEstimator.allocateResourcesAndEstimateCost(region, 1)
+ val (resourceConfig, regionCost) = costEstimatorMemoization
Review Comment:
When do we clean this cache?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(amber): add memoization of region costs in CostBasedScheduleGenerator [texera]
Posted by "Xiao-zhen-Liu (via GitHub)" <gi...@apache.org>.
Xiao-zhen-Liu merged PR #3660:
URL: https://github.com/apache/texera/pull/3660
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(amber): add memoization of region costs in CostBasedScheduleGenerator [texera]
Posted by "Xiao-zhen-Liu (via GitHub)" <gi...@apache.org>.
Xiao-zhen-Liu commented on code in PR #3660:
URL: https://github.com/apache/texera/pull/3660#discussion_r2302317877
##########
core/amber/src/main/scala/edu/uci/ics/amber/engine/architecture/scheduling/CostBasedScheduleGenerator.scala:
##########
@@ -75,6 +76,17 @@ class CostBasedScheduleGenerator(
actorId = actorId
)
+ private case class CostEstimatorMemoKey(
+ physicalOpIds: Set[PhysicalOpIdentity],
+ physicalLinkIds: Set[PhysicalLink],
+ portIds: Set[GlobalPortIdentity],
+ resourceConfig: Option[ResourceConfig]
Review Comment:
No, the current resource allocator needs to check the existing ResourceConfig of the region so this needs to be provided.
##########
core/amber/src/main/scala/edu/uci/ics/amber/engine/architecture/scheduling/CostBasedScheduleGenerator.scala:
##########
@@ -75,6 +76,17 @@ class CostBasedScheduleGenerator(
actorId = actorId
)
+ private case class CostEstimatorMemoKey(
+ physicalOpIds: Set[PhysicalOpIdentity],
+ physicalLinkIds: Set[PhysicalLink],
+ portIds: Set[GlobalPortIdentity],
Review Comment:
Yes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(amber): add memoization of region costs in CostBasedScheduleGenerator [texera]
Posted by "Xiao-zhen-Liu (via GitHub)" <gi...@apache.org>.
Xiao-zhen-Liu commented on code in PR #3660:
URL: https://github.com/apache/texera/pull/3660#discussion_r2299644976
##########
core/amber/src/main/scala/edu/uci/ics/amber/engine/architecture/scheduling/CostBasedScheduleGenerator.scala:
##########
@@ -606,8 +610,8 @@ class CostBasedScheduleGenerator(
.map(level =>
level
.map(region => {
- val (resourceConfig, regionCost) =
- costEstimator.allocateResourcesAndEstimateCost(region, 1)
+ val (resourceConfig, regionCost) = costEstimatorMemoization
Review Comment:
It has the same lifecycle as `CostBasedScheduleGenerator` and it needs to be used throughout the search process. As a result it is cleaned after the scheduler generates a schedule.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(amber): add memoization of region costs in CostBasedScheduleGenerator [texera]
Posted by "Xiao-zhen-Liu (via GitHub)" <gi...@apache.org>.
Xiao-zhen-Liu commented on code in PR #3660:
URL: https://github.com/apache/texera/pull/3660#discussion_r2299674656
##########
core/amber/src/main/scala/edu/uci/ics/amber/engine/architecture/scheduling/CostBasedScheduleGenerator.scala:
##########
@@ -606,8 +610,8 @@ class CostBasedScheduleGenerator(
.map(level =>
level
.map(region => {
- val (resourceConfig, regionCost) =
- costEstimator.allocateResourcesAndEstimateCost(region, 1)
+ val (resourceConfig, regionCost) = costEstimatorMemoization
+ .getOrElseUpdate(region, costEstimator.allocateResourcesAndEstimateCost(region, 1))
Review Comment:
We don't, but we do need all the Ids in the region (except regionId) to uniquely identify a region so that it contains all the information needed by the costEstimator and resourceAllocator to distinguish two regions with potentially different outputs of `costEstimator.allocateResourcesAndEstimateCost`. I created a separate case class for this key.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(amber): add memoization of region costs in CostBasedScheduleGenerator [texera]
Posted by "Xiao-zhen-Liu (via GitHub)" <gi...@apache.org>.
Xiao-zhen-Liu commented on code in PR #3660:
URL: https://github.com/apache/texera/pull/3660#discussion_r2299674656
##########
core/amber/src/main/scala/edu/uci/ics/amber/engine/architecture/scheduling/CostBasedScheduleGenerator.scala:
##########
@@ -606,8 +610,8 @@ class CostBasedScheduleGenerator(
.map(level =>
level
.map(region => {
- val (resourceConfig, regionCost) =
- costEstimator.allocateResourcesAndEstimateCost(region, 1)
+ val (resourceConfig, regionCost) = costEstimatorMemoization
+ .getOrElseUpdate(region, costEstimator.allocateResourcesAndEstimateCost(region, 1))
Review Comment:
We don't, but we do need all the Ids in the region (except regionId) to uniquely identify a region so that it contains all the information needed by the costEstimator and resourceAllocator. I created a separate case class for this key.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(amber): add memoization of region costs in CostBasedScheduleGenerator [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on code in PR #3660:
URL: https://github.com/apache/texera/pull/3660#discussion_r2301025348
##########
core/amber/src/main/scala/edu/uci/ics/amber/engine/architecture/scheduling/CostBasedScheduleGenerator.scala:
##########
@@ -75,6 +76,17 @@ class CostBasedScheduleGenerator(
actorId = actorId
)
+ private case class CostEstimatorMemoKey(
+ physicalOpIds: Set[PhysicalOpIdentity],
+ physicalLinkIds: Set[PhysicalLink],
+ portIds: Set[GlobalPortIdentity],
+ resourceConfig: Option[ResourceConfig]
Review Comment:
This should be part of the value, not the key?
##########
core/amber/src/main/scala/edu/uci/ics/amber/engine/architecture/scheduling/CostBasedScheduleGenerator.scala:
##########
@@ -75,6 +76,17 @@ class CostBasedScheduleGenerator(
actorId = actorId
)
+ private case class CostEstimatorMemoKey(
+ physicalOpIds: Set[PhysicalOpIdentity],
+ physicalLinkIds: Set[PhysicalLink],
+ portIds: Set[GlobalPortIdentity],
Review Comment:
We are removing portIds in the region right? Just want to confirm we don't need this in the future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org