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.

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

Jump to bottom

Include memory access costs in cost models (cost-based optimizer) #2397

Merged
merged 4 commits into from
May 20, 2021
Merged

Include memory access costs in cost models (cost-based optimizer) #2397

merged 4 commits into from
May 20, 2021

Conversation

andygrove
Copy link
Contributor

@andygrove andygrove commented May 11, 2021 •
edited
Loading

Signed-off-by: Andy Grove andygrove@nvidia.com

This PR starts to introduce the mechansim for including memory access costs in the cost model. The default settings are 320 GB/s for GPU and 30 GB/s for CPU.

This also contains a fix for a bug that was highlighted during testing where CBO could move a BroadcastExchange back to CPU but leave the BHJ on GPU, resulting in an invalid plan.

@andygrove
Copy link
Contributor Author

@revans2 This is what I have so far for incorporating memory access costs. It's very early and I have plenty to fix already but wanted to get your feedback early on to make sure I am heading in the right direction. I will handle kernel costs in a separate PR.

@sameerz sameerz added the feature request New feature or request label May 12, 2021
@andygrove andygrove force-pushed the mem-access-part-one branch from eb9be1f to c979e95 Compare May 13, 2021 14:58
revans2
revans2 previously approved these changes May 13, 2021
View reviewed changes
@revans2
Copy link
Collaborator

revans2 commented May 13, 2021

@revans2 This is what I have so far for incorporating memory access costs. It's very early and I have plenty to fix already but wanted to get your feedback early on to make sure I am heading in the right direction. I will handle kernel costs in a separate PR.

Jason had a good point when we talked that it is not really kernel costs as much as it is a fixed cost per operation. The cost of launching a kernel is likely small compared to other parts of the computation.

@andygrove andygrove changed the title WIP: Partial support for memory access costs Include memory access costs in cost models (cost-based optimizer) May 13, 2021
@andygrove andygrove marked this pull request as ready for review May 13, 2021 22:08
@andygrove
Copy link
Contributor Author

@revans2 This is ready for review now.

@andygrove andygrove mentioned this pull request May 14, 2021
Open
@andygrove
Copy link
Contributor Author

I filed a follow-on issue for improving our handling of BHJ: #2417

revans2
revans2 previously approved these changes May 17, 2021
View reviewed changes
Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

This looks fine to me, but I would like to hear from others

@andygrove
Copy link
Contributor Author

I filed #2430 for adding kernel launch costs

@andygrove
Copy link
Contributor Author

build

andygrove added 4 commits May 17, 2021 13:39
@andygrove
Implement basic mechanism for memory access costs
c1e3e0c 
Signed-off-by: Andy Grove <andygrove@nvidia.com>
@andygrove
Use real memory speeds in model
efe00a3
@andygrove
Fix bug with moving broadcast exchange back to CPU
2188b0e 
Signed-off-by: Andy Grove <andygrove@nvidia.com>
@andygrove
Remove redundant configs
a62289f 
Signed-off-by: Andy Grove <andygrove@nvidia.com>
@andygrove andygrove force-pushed the mem-access-part-one branch from ed03490 to a62289f Compare May 17, 2021 19:39
@andygrove
Copy link
Contributor Author

Rebased to pull in fix for test failure

@andygrove
Copy link
Contributor Author

build

@andygrove andygrove mentioned this pull request May 18, 2021
Open
@pxLi pxLi changed the base branch from branch-0.6 to branch-21.06 May 19, 2021 01:11
@andygrove andygrove linked an issue May 20, 2021 that may be closed by this pull request
[FEA] CBO: Implement costs for memory access and launching kernels #2367
Closed
@andygrove
Copy link
Contributor Author

@revans2 Any objection to me merging this now so we can see the impact in the automated benchmarks?

@andygrove andygrove merged commit e1b493c into NVIDIA:branch-21.06 May 20, 2021
@andygrove andygrove deleted the mem-access-part-one branch May 20, 2021 19:55
abellina pushed a commit to abellina/spark-rapids that referenced this pull request May 24, 2021
@andygrove @abellina
Include memory access costs in cost models (cost-based optimizer) (NV…
e06b5aa 
…IDIA#2397)

* Implement basic mechanism for memory access costs

Signed-off-by: Andy Grove <andygrove@nvidia.com>

* Use real memory speeds in model

* Fix bug with moving broadcast exchange back to CPU

Signed-off-by: Andy Grove <andygrove@nvidia.com>

* Remove redundant configs

Signed-off-by: Andy Grove <andygrove@nvidia.com>
nartal1 pushed a commit to nartal1/spark-rapids that referenced this pull request Jun 9, 2021
@andygrove
Include memory access costs in cost models (cost-based optimizer) (NV…
13aff05 
…IDIA#2397)

* Implement basic mechanism for memory access costs

Signed-off-by: Andy Grove <andygrove@nvidia.com>

* Use real memory speeds in model

* Fix bug with moving broadcast exchange back to CPU

Signed-off-by: Andy Grove <andygrove@nvidia.com>

* Remove redundant configs

Signed-off-by: Andy Grove <andygrove@nvidia.com>
nartal1 pushed a commit to nartal1/spark-rapids that referenced this pull request Jun 9, 2021
@andygrove
Include memory access costs in cost models (cost-based optimizer) (NV…
fcd9153 
…IDIA#2397)

* Implement basic mechanism for memory access costs

Signed-off-by: Andy Grove <andygrove@nvidia.com>

* Use real memory speeds in model

* Fix bug with moving broadcast exchange back to CPU

Signed-off-by: Andy Grove <andygrove@nvidia.com>

* Remove redundant configs

Signed-off-by: Andy Grove <andygrove@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@revans2 revans2 revans2 approved these changes

Assignees
No one assigned
Labels
feature request New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] CBO: Implement costs for memory access and launching kernels
3 participants
@andygrove @revans2 @sameerz