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

Free temp memory no longer needed in multibyte_split processing #16091

Merged
merged 29 commits into from
Jul 9, 2024

Conversation

davidwendt
Copy link
Contributor

@davidwendt davidwendt commented Jun 25, 2024

Description

Updates the multibyte_split logic to free temporary memory once the chars and offsets have been resolved. This gives room to the remaining processing if more temp memory is required.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@davidwendt davidwendt added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jun 25, 2024
@davidwendt davidwendt self-assigned this Jun 25, 2024
@github-actions github-actions bot added the CMake CMake build issue label Jun 25, 2024
@github-actions github-actions bot removed the CMake CMake build issue label Jul 1, 2024
@davidwendt
Copy link
Contributor Author

The change is best viewed with the "Hide whitespace" option. A main section of the source was converted to a IILE which mainly indents many of the lines into a new bracket { } scope.
github-pr-hide-whitespace

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Jul 1, 2024
@davidwendt davidwendt marked this pull request as ready for review July 1, 2024 23:12
@davidwendt davidwendt requested a review from a team as a code owner July 1, 2024 23:12
@davidwendt davidwendt requested review from vyasr and nvdbaranec July 1, 2024 23:12
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Looks like most of the diff is code movement/indentation. It looks like the only relevant change is scoping, to allow freeing the temporaries. This looks fine.

Comment on lines 58 to 60
namespace cudf {
namespace io {
namespace text {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like combining these, it tells me more about the scope of this file (specifically that we won't be putting things into cudf::io or cudf, just cudf::io::text.

Suggested change
namespace cudf {
namespace io {
namespace text {
namespace cudf::io::text {

@bdice
Copy link
Contributor

bdice commented Jul 2, 2024

The change is best viewed with the "Hide whitespace" option.

I saw this comment just a bit too late! Thanks for the heads-up.

Copy link
Contributor

@nvdbaranec nvdbaranec left a comment

Choose a reason for hiding this comment

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

Only comment I have is that this function is fairly bulky. Might be cleaner to separate this code out into it's own function instead of embedded as a lambda. Minor point though.

@davidwendt
Copy link
Contributor Author

Only comment I have is that this function is fairly bulky. Might be cleaner to separate this code out into it's own function instead of embedded as a lambda. Minor point though.

Yes, I usually prefer that approach myself but the number of parameters passed in and out seemed like too much here.

@davidwendt
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 433e959 into rapidsai:branch-24.08 Jul 9, 2024
79 checks passed
@davidwendt davidwendt deleted the mbs-free-temp-mem branch July 9, 2024 14:45
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 improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants