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

fix: Remove same page hack for simdStrstr #11553

Closed
wants to merge 1 commit into from

Conversation

Yuhta
Copy link
Contributor

@Yuhta Yuhta commented Nov 15, 2024

Summary:
The hack is breaking ASAN check, removing it does not give huge
performance loss though so we remove it.

Benchmark results before vs after change:
https://gist.github.com/Yuhta/b3f66e85d52b62240c09c15d6e7941bb

Differential Revision: D66012678

Summary:
The hack is breaking ASAN check, removing it does not give huge
performance loss though so we remove it.

Benchmark results before vs after change:
https://gist.github.com/Yuhta/b3f66e85d52b62240c09c15d6e7941bb

Differential Revision: D66012678
@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 Nov 15, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D66012678

Copy link

netlify bot commented Nov 15, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit dfd6085
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/67379973f8e936000873c0c5

Copy link
Contributor

@kevinwilfong kevinwilfong left a comment

Choose a reason for hiding this comment

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

LGTM , thanks JImmy!

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 0a422f7.

Copy link

Conbench analyzed the 1 benchmark run on commit 0a422f73.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@skadilover
Copy link
Contributor

@Yuhta

This may not guarantee that simdstrstr will always be faster than std in most cases. Doing so will always lose the last batch.

tpchQuery13 54.72ms 18.28

tpchQuery13 27.57ms 36.27

@Yuhta
Copy link
Contributor Author

Yuhta commented Dec 3, 2024

@skadilover Are we slower than std in such cases? It probably should not be because the fallback is std.

I tried a few annotations to skip ASAN on the memory load without success, maybe you can give it a try when you have time.

@skadilover
Copy link
Contributor

skadilover commented Dec 4, 2024

@skadilover Are we slower than std in such cases? It probably should not be because the fallback is std.

I tried a few annotations to skip ASAN on the memory load without success, maybe you can give it a try when you have time.

@Yuhta
I am trying to use simdstrstr to replace std::find in Like match funciton : matchSubstringsPattern, I found that the perfomance of TPCH-Q13 in LikeTpchBenchmark.cpp is not improved, so I tried revert the change of this MR

now :
tpchQuery13 54.72ms 18.28
revert :
tpchQuery13 27.57ms 36.27

Q13`s string-filter is the case that unmatch more frequently

@skadilover
Copy link
Contributor

skadilover commented Dec 4, 2024

I tried a few annotations to skip ASAN on the memory load without success

So you mean the code before will trigger ASAN check failed ?

maybe you can give it a try when you have time

Could you detail how to reproduce it? Share the compile flag you used ?

I tried add -fsanitize=address, LikeTpchBenchmark.cpp will always failed (remove or not remove the hack code), seems nothing to do with this:

image

@skadilover
Copy link
Contributor

@Yuhta
Do you see the error like this ?

image

@Yuhta
Copy link
Contributor Author

Yuhta commented Dec 4, 2024

@skadilover Yes it's the xsimd load_unaligned that triggers ASAN

athmaja-n pushed a commit to athmaja-n/velox that referenced this pull request Jan 10, 2025
Summary:
Pull Request resolved: facebookincubator#11553

The hack is breaking ASAN check, removing it does not give huge
performance loss though so we remove it.

Benchmark results before vs after change:
https://gist.github.com/Yuhta/b3f66e85d52b62240c09c15d6e7941bb

Reviewed By: kevinwilfong

Differential Revision: D66012678

fbshipit-source-id: 107ae9e715ff5edd20707fb49b970ae5d3190559
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants