Skip to content

Faster validation logic in bc_str2num() #14115

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

Merged
merged 1 commit into from
May 3, 2024

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented May 2, 2024

Using SIMD to accelerate the validation.

Using the benchmark from #14076.

After:

1.3504369258881
1.6206321716309
1.6845638751984

Before:

1.4750170707703
1.9039781093597
1.9632289409637

Using SIMD to accelerate the validation.

Using the benchmark from php#14076.

After:
```
1.3504369258881
1.6206321716309
1.6845638751984
```

Before:
```
1.4750170707703
1.9039781093597
1.9632289409637
```
@SakiTakamachi
Copy link
Member

LGTM, Your tricks always amaze me!

One suggestion: what about implementing this in zend instead of BCMath?

For example, ctype_digit checks if a string is a digit in exactly the same way as the existing BCMath does.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

I am not going to claim I understand the SSE intrinsic code, but I'm trusting this is fine.

@SakiTakamachi SakiTakamachi mentioned this pull request May 3, 2024
@nielsdos nielsdos merged commit cad0e55 into php:master May 3, 2024
10 checks passed
@nielsdos
Copy link
Member Author

nielsdos commented May 3, 2024

One suggestion: what about implementing this in zend instead of BCMath?

For example, ctype_digit checks if a string is a digit in exactly the same way as the existing BCMath does.

Maybe, although isdigit takes into account the locale and I'm not sure if all locales have the same digit range.

@Girgias
Copy link
Member

Girgias commented May 4, 2024

Seems like isdigit is not affected by the locale according to the note: https://devdocs.io/c/string/byte/isdigit

@nielsdos
Copy link
Member Author

nielsdos commented May 4, 2024

Interesting, in that case someone may feel free to move it somewhere more generally such that it can be reused.

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

Successfully merging this pull request may close these issues.

3 participants