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

Refactor setting stack size in regex code #8358

Merged
merged 10 commits into from
Jun 7, 2021

Conversation

davidwendt
Copy link
Contributor

This PR is the first of several to cleanup the regex strings code. The regex code employs multiple stack sizes to perform its matching based on the number of instructions in the given regex pattern. The current implementation allocates the stack for the regex code. This PR moves this down into the regex functions themselves. This helps simplify the interface and reduce compile time a bit.

@davidwendt davidwendt added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. strings strings issues (C++ and Python) improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 25, 2021
@davidwendt davidwendt self-assigned this May 25, 2021
@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels May 26, 2021
@davidwendt davidwendt marked this pull request as ready for review May 26, 2021 15:17
@davidwendt davidwendt requested a review from a team as a code owner May 26, 2021 15:17
@davidwendt davidwendt requested review from devavret and nvdbaranec May 26, 2021 15:17
@nvdbaranec
Copy link
Contributor

Question about the transition between "LARGE" stack size and "global memory". It seems like there's a hole between RX_LARGE_INSTS (920) and MAX_STACK_INSTS (1000). It looks like this code can get into the RX_STACK_ANY case as soon as the # of instructions is > 920. But the code for allocating the global memory doesn't kick in until 1000 instructions:

  if (insts_count > MAX_STACK_INSTS) {
    auto relist_alloc_size = relist::alloc_size(insts_count);
    rlm_size               = relist_alloc_size * 2L * strings_count;  // reljunk has 2 relist ptrs
  }

Is there some kind of rounding thing going on here where it will always be some safe multiple of instructions?

@davidwendt
Copy link
Contributor Author

I think you are right. There appears to be an overlap where too much memory is being allocated. I should change the threshold check to use RX_LARGE_INSTS instead of MAX_STACK_INSTS and then get rid of the latter.

@codecov
Copy link

codecov bot commented Jun 2, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.08@606d854). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.08    #8358   +/-   ##
===============================================
  Coverage                ?   82.83%           
===============================================
  Files                   ?      109           
  Lines                   ?    17901           
  Branches                ?        0           
===============================================
  Hits                    ?    14828           
  Misses                  ?     3073           
  Partials                ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 606d854...5aef511. Read the comment docs.

Copy link
Contributor

@devavret devavret left a comment

Choose a reason for hiding this comment

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

Didn't review the entire logic but now I understand the change. Code quality looks good.

@davidwendt
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit ff1e849 into rapidsai:branch-21.08 Jun 7, 2021
@davidwendt davidwendt deleted the refactor-regex-stack branch June 7, 2021 19:47
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 strings strings issues (C++ and Python)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants