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

Improve performance of nvtext::tokenize_with_vocabulary for long strings #14336

Merged
merged 11 commits into from
Nov 3, 2023
Merged

Improve performance of nvtext::tokenize_with_vocabulary for long strings #14336

merged 11 commits into from
Nov 3, 2023

Conversation

davidwendt
Copy link
Contributor

Description

Improves nvtext::tokenize_with_vocabulary performance for long strings. Also adds additional tests and an nvbench benchmark.

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. strings strings issues (C++ and Python) improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Oct 26, 2023
@davidwendt davidwendt self-assigned this Oct 26, 2023
@github-actions github-actions bot added the CMake CMake build issue label Oct 26, 2023
@davidwendt
Copy link
Contributor Author

Performance numbers for long strings from the included benchmark

| width |  num_rows  |   Ref Time |   Cmp Time |          Diff |   %Diff |      |
|-------|------------|------------|------------|---------------|---------|------|
|  256  |   262144   |   3.267 ms |   1.694 ms |  -1572.363 us | -48.13% | 1.93 |
|  512  |   262144   |   8.746 ms |   3.193 ms |  -5553.180 us | -63.49% | 2.74 |
| 1024  |   262144   |  20.265 ms |   6.129 ms | -14135.536 us | -69.75% | 3.31 |
|  256  |   524288   |   6.344 ms |   3.326 ms |  -3018.149 us | -47.57% | 1.91 |
|  512  |   524288   |  18.740 ms |   6.387 ms | -12352.539 us | -65.92% | 2.93 |
| 1024  |   524288   |  42.879 ms |  12.394 ms | -30485.431 us | -71.10% | 3.46 |
|  256  |  1048576   |  11.448 ms |   6.693 ms |  -4755.292 us | -41.54% | 1.71 |
|  512  |  1048576   |  34.554 ms |  12.878 ms | -21676.282 us | -62.73% | 2.68 |
| 1024  |  1048576   |  80.785 ms |  24.909 ms | -55876.306 us | -69.17% | 3.24 |
|  256  |  2097152   |  23.507 ms |  13.482 ms | -10025.858 us | -42.65% | 1.74 |
|  512  |  2097152   |  69.627 ms |  26.090 ms | -43537.188 us | -62.53% | 2.67 |
|  256  |  4194304   |  45.730 ms |  27.060 ms | -18669.250 us | -40.83% | 1.69 |

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Oct 27, 2023
@davidwendt davidwendt marked this pull request as ready for review October 31, 2023 21:02
@davidwendt davidwendt requested a review from a team as a code owner October 31, 2023 21:02
bdice
bdice approved these changes Oct 31, 2023
View reviewed changes
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.

All looks good. I have one suggested rewrite for some of the counting math.

cpp/src/text/vocabulary_tokenize.cu Outdated Show resolved Hide resolved
mythrocks
mythrocks approved these changes Nov 3, 2023
View reviewed changes
Copy link
Contributor

@mythrocks mythrocks left a comment

Choose a reason for hiding this comment

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

Thanks for the comment and explanation. +1.

@davidwendt
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit f97e74f into rapidsai:branch-23.12 Nov 3, 2023
61 checks passed
@davidwendt davidwendt deleted the vocab-tokenize-perf branch November 3, 2023 19:58
@davidwendt davidwendt mentioned this pull request Nov 10, 2023
Merged
3 tasks
rapids-bot bot pushed a commit that referenced this pull request Nov 14, 2023
@davidwendt
Fix token-count logic in nvtext::tokenize_with_vocabulary (#14393)
b446a6f 
Fixes a bug introduced in #14336 when trying to simplify the token-counting logic as per this discussion #14336 (comment)
The simplification caused an error which was found when running the nvtext benchmarks.
The appropriate gtest has been updated to cover this case now.

Authors:
  - David Wendt (https://github.com/davidwendt)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Karthikeyan (https://github.com/karthikeyann)

URL: #14393
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdice bdice bdice approved these changes

@mythrocks mythrocks mythrocks approved these changes

@divyegala divyegala Awaiting requested review from divyegala divyegala is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

Assignees

@davidwendt davidwendt

Labels
3 - Ready for Review Ready for review by team CMake CMake build issue 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
cuDF/Dask/Numba/UCX
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@davidwendt @bdice @mythrocks