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

prevent use ob mb_* functions on non-multibyte strings - 2,8% perf gain #30

Closed
wants to merge 8 commits into from

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Mar 13, 2021

this PR does 2 things

  • use mb_* functions only, when the string beeing streamed contains multibyte chars. that way we can use way faster non mb-string functions on non-multibyte strings
  • use full-qualified call to string and multibyte-string functions, which allows the php parser to apply compile time optimizations when strings are known (e.g. string literals)

grafik

@staabm staabm changed the title prevent use ob mb_* functions on non-multibyte strings prevent use ob mb_* functions on non-multibyte strings - 2,8% perf gain Mar 13, 2021
This reverts commit 3c26a95.
@staabm staabm marked this pull request as ready for review March 13, 2021 10:21
@turanct
Copy link
Collaborator

turanct commented Mar 13, 2021

I quickly implemented a second string stream which doesn't work with mb_* functions, but just uses the default string manipulations. This indeed gives us quite a nice speed improvement (much better even than the 2.8% you measured):

Screenshot 2021-03-13 at 22 03 11

This also gives the user more control over which implementation is used. I'm not sure if we should risk weird behavior during parsing by automatically switching functions based on the byte length of the input. If we do, we should probably have some proper tests to cover for this.

I'll be running some experiments on this, to see what the best solution would be.

thanks a lot!

@staabm
Copy link
Contributor Author

staabm commented Mar 13, 2021

Btw. Just found #28 which implements a similar approach

@turanct
Copy link
Collaborator

turanct commented Mar 17, 2021

closing in favor of #28

I would like to have the user explicitly chose which type of stream they provide, to make sure we don't have unexpected behavior within StringStream

@turanct turanct closed this Mar 17, 2021
@staabm staabm deleted the no_mb branch March 17, 2021 20:37
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.

2 participants