Skip to content

Revert "New Decimal <--> Floating conversion (#15905)" - #16283

Closed
ttnghia wants to merge 2 commits into
NVIDIA:branch-24.08from
ttnghia:revert_15905
Closed

Revert "New Decimal <--> Floating conversion (#15905)"#16283
ttnghia wants to merge 2 commits into
NVIDIA:branch-24.08from
ttnghia:revert_15905

Conversation

@ttnghia

@ttnghia ttnghia commented Jul 15, 2024

Copy link
Copy Markdown
Contributor

After the PR #15905 merged, there was a performance regression reported:

We observed significant performance degradation to binary ops. We see 12-14x slowdowns in our AST benchmarks, plus 6-10x slowdowns in other binary ops.

This reverts the problematic PR (#15905), waiting for more investigation on the regression.

@ttnghia ttnghia added 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. Performance Performance related issue non-breaking Non-breaking change labels Jul 15, 2024
@ttnghia ttnghia self-assigned this Jul 15, 2024
@ttnghia
ttnghia requested review from a team as code owners July 15, 2024 19:51
@github-actions github-actions Bot added Python Affects Python cuDF API. Java Affects Java cuDF API. labels Jul 15, 2024
@ttnghia ttnghia added the bug Something isn't working label Jul 15, 2024

@vyasr vyasr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving based on offline discussions indicating that we think it's safe to go ahead without rebenchmarking.

@ttnghia

ttnghia commented Jul 15, 2024

Copy link
Copy Markdown
Contributor Author

/merge

@ttnghia ttnghia added the DO NOT MERGE Hold off on merging; see PR for details label Jul 16, 2024
@abellina

Copy link
Copy Markdown
Contributor

@ttnghia just wondering what we are waiting for in this PR.

@ttnghia

ttnghia commented Jul 19, 2024

Copy link
Copy Markdown
Contributor Author

@ttnghia just wondering what we are waiting for in this PR.

There seems to be another fix merged in cudf so this may no longer be needed. I'm still waiting for confirmation before closing this.

@mattahrens

Copy link
Copy Markdown

This is the related perf fix PR: #16287. It's been merged, but still waiting for final confirmation that is fixed the issue.

@ttnghia ttnghia closed this Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3 - Ready for Review Ready for review by team bug Something isn't working DO NOT MERGE Hold off on merging; see PR for details Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Performance Performance related issue Python Affects Python cuDF API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants