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

Utf8 valid arm64 neon #87

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

AndreyyTs
Copy link

@AndreyyTs AndreyyTs commented Nov 3, 2024

Successfully implemented ARM optimization analogous to AVX2 vectorization, following the methodology outlined in the research paper (arxiv.org/pdf/2010.03090)

  • Validation: All test suites executed successfully
  • Performance Metrics: Achieved up to 9x performance improvement in optimal scenarios

Benchmark Results

Environment

  • OS: darwin
  • Architecture: arm64
  • Package: github.com/segmentio/asm/utf8
  • CPU: Apple M1 Pro

Results Table

Test Case Implementation Operations Time (ns/op) Throughput (MB/s)
1kValid NEON 10,070,200 106.4 9,626.33
1kValid Stdlib 1,334,408 902.9 1,134.08
1MValid Stdlib 1,298 919,417 1,140.48
1MValid NEON 10,000 103,341 10,146.78
10ASCII NEON 239,135,683 5.061 1,975.71
10ASCII Stdlib 316,728,998 3.795 2,634.98
10Japan NEON 36,628,311 32.49 923.25
10Japan Stdlib 41,826,722 28.29 1,060.37
small0 NEON 272,401,705 4.407 -
small0 Stdlib 545,493,081 2.196 -
small4 NEON 180,697,672 6.591 606.89
small4 Stdlib 346,882,966 3.473 1,151.63
small8 Stdlib 195,698,066 6.160 1,298.67
small8 NEON 131,638,969 9.150 874.36

Your Name added 4 commits November 3, 2024 01:22
@seg-atlantis-prod
Copy link

Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors

@seg-atlantis-prod
Copy link

Error parsing command: EOF found when expecting closing quote

@seg-atlantis-prod
Copy link

Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors

@seg-atlantis-prod
Copy link

Error parsing command: EOF found when expecting closing quote

@achille-roussel
Copy link
Contributor

This is fantastic!

Unfortunately I don't have permissions to merge on this repository anymore, but maybe @kevinburkesegment can give a hand?

@AndreyyTs
Copy link
Author

@achille-roussel
Thank you, I would like to help the project.
But I need to refactor my code. Also, I don't know which license I should specify. This is my first experience in open source

@achille-roussel
Copy link
Contributor

The project is published under MIT license, so your contributions to it will be licensed the same way, there are no other actions needed on your end.

Copy link
Contributor

@kalamay kalamay left a comment

Choose a reason for hiding this comment

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

@AndreyyTs thanks so much for the contribution! Yep, we don't really need much for an additional license here, though we should probably adopt using an SPDX comment in each file.

I've left a few minor comments, but overall this looks great!

utf8/test.out.txt Outdated Show resolved Hide resolved
utf8/valid_arm64.s Outdated Show resolved Hide resolved
@seg-atlantis-prod
Copy link

Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors

@seg-atlantis-prod
Copy link

Error parsing command: EOF found when expecting closing quote

@AndreyyTs AndreyyTs force-pushed the utf8_valid_arm64_NEON branch from e34f163 to 53e0bdb Compare November 7, 2024 10:37
@seg-atlantis-prod
Copy link

Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors

@seg-atlantis-prod
Copy link

Error parsing command: EOF found when expecting closing quote

@AndreyyTs
Copy link
Author

@kalamay
I made the changes. Do I need to do anything else?
Also, I could make more optimizations if there are any suggestions.

@AndreyyTs AndreyyTs requested a review from kalamay November 7, 2024 12:45
@seg-atlantis-prod
Copy link

Atlantis commands can't be run on fork pull requests. To enable, set --allow-fork-prs or, to disable this message, set --silence-fork-pr-errors

@seg-atlantis-prod
Copy link

Error parsing command: EOF found when expecting closing quote

@kalamay
Copy link
Contributor

kalamay commented Nov 13, 2024

Hi @AndreyyTs, I'm trying to set aside some time to figure out why this is impacting our builds on the amd64 targets, but I just haven't quite had a free moment! I'll get to take a more detailed look this week.

@AndreyyTs
Copy link
Author

@kalamay Hi, did you manage to watch it?

@kalamay
Copy link
Contributor

kalamay commented Nov 27, 2024

So I've dug into a bit more, and it looks like the issue is with the use of go version 1.18 as a minimum version. It seems to lack definitions for some of the NEON instructions you are using. In this case, the first instruction that is unrecognized looks to be ADR in utf8/valid_arm64.s on line 112. To be clear, this isn't that the code is wrong, it's that the go compiler version doesn't recognize it. This has been an issue we've hit in the past, especially when targeting ARM.

You can see a few more details here: https://github.com/segmentio/asm/actions/runs/11741614998/job/33625198406?pr=87

There's generally two ways around this. We can bump up the minimum version, but we need to figure out which version to target. This also needs to be considered, as this does then have downstream affects on minimum versions. Do you have an idea what version you were using when developing this?

The second way around this is to directly encode the instructions as binary instead of using assembly mnemonics. For example, I've does this in our base64 here: https://github.com/segmentio/asm/blob/main/base64/encode_arm64.s#L59

@AndreyyTs
Copy link
Author

@kalamay
Hello I used Go version 1.23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants