-
Notifications
You must be signed in to change notification settings - Fork 922
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
Optimize URL Decoding #8622
Optimize URL Decoding #8622
Conversation
…ptimize-url-decode
…optimize-url-decode
Codecov Report
@@ Coverage Diff @@
## branch-21.10 #8622 +/- ##
===============================================
Coverage ? 10.75%
===============================================
Files ? 114
Lines ? 18695
Branches ? 0
===============================================
Hits ? 2010
Misses ? 16685
Partials ? 0 Continue to review full report at Codecov.
|
Hi @gaohao95 -- do you think this PR will be completed in time for 21.08 code freeze (July 22nd?). Wondering where this should go on the project board. Thanks! |
Hi @shwina, this PR is motivated by spark's string performance and I think the PR itself is more-or-less ready. But the last time I shared the code with @chenrui17, they reported a unit test failure on their ends. After that, I have discovered and fixed a bug, but I am unsure whether the fix solves their test failures. I think we should merge this only after we hear back from them. Loop in @jlowe and @chenrui17 to see whether they have any feedbacks on priority. |
…optimize-url-decode
@chenrui17 reported that the unit test can pass at their end now. I think this PR is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, the perf improvements look great. Some minor comments, a few of which are more for my edification than anything else.
…optimize-url-decode
…optimize-url-decode
Hi @vyasr @davidwendt, I think all comments have been addressed. Could you take another look to see whether it's ready to merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The kernel logic handling of null entries needs to be corrected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work.
@gpucibot merge |
This PR is intended to optimize the URL decoding performance, especially on large URLs. Additionally, a test case for large URLs has been added.
When tested on V100, baseline performance at 7521c3f
This PR
close #8030