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

refactor: IntDecoder #11790

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: IntDecoder #11790

wants to merge 1 commit into from

Conversation

zuyu
Copy link
Contributor

@zuyu zuyu commented Dec 9, 2024 •
edited
Loading

  • optimize the isSigned case into constexpr
  • reduce unnecessary variables in loops
  • allow fast reading smaller size integers by checking bufferEnd_ w/ numBytes_

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 9, 2024
@netlify Netlify
Copy link

netlify bot commented Dec 9, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit e4cd075
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6764f99a38b7ac0008f725e7

@zuyu zuyu force-pushed the int-decoder-refactor branch from 772f458 to 5669bfd Compare December 9, 2024 03:37
Yuhta
Yuhta reviewed Dec 9, 2024
View reviewed changes
velox/dwio/common/IntDecoder.h Outdated Show resolved Hide resolved
velox/dwio/common/IntDecoder.h Outdated
}
return readVsLong();
Copy link
Contributor

Choose a reason for hiding this comment

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

The old code just seems better, generating smaller binary and reducing compilation time.

velox/dwio/common/IntDecoder.h Outdated Show resolved Hide resolved
velox/dwio/common/IntDecoder.h Outdated Show resolved Hide resolved
velox/dwio/common/IntDecoder.h Outdated Show resolved Hide resolved
@zuyu zuyu force-pushed the int-decoder-refactor branch 3 times, most recently from 081374e to ab2d605 Compare December 10, 2024 15:45
@zuyu zuyu marked this pull request as ready for review December 10, 2024 15:46
@zuyu zuyu requested a review from Yuhta December 10, 2024 17:38
@zuyu zuyu force-pushed the int-decoder-refactor branch from ab2d605 to d28c180 Compare December 10, 2024 17:59
@zuyu
Copy link
Contributor Author

zuyu commented Dec 11, 2024

Encountered Spark flaky fuzzer test per #11601: https://github.com/facebookincubator/velox/actions/runs/12261951138/job/34213130214?pr=11790

@zuyu zuyu force-pushed the int-decoder-refactor branch from d28c180 to 87ce0ee Compare December 11, 2024 15:18
@zuyu zuyu force-pushed the int-decoder-refactor branch from 87ce0ee to e4cd075 Compare December 20, 2024 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yuhta Yuhta Awaiting requested review from Yuhta

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zuyu @Yuhta @facebook-github-bot