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 for very slow chunk processing for larger content in the http res… #354

Conversation

aleksandar-mitrev
Copy link

@aleksandar-mitrev aleksandar-mitrev commented Jun 13, 2023

@thomaslauria
Copy link

Additional information:

  • Original code needs about 40 minutes to process a 140MB chunked HTTP response
  • Fixed code needs just a second

// If mbstring overloads substr and strlen functions, we have to
// override it's internal encoding
if (function_exists('mb_internal_encoding') &&
((int) ini_get('mbstring.func_overload')) & 2) {

Choose a reason for hiding this comment

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

mbstring.func_overload was deprecated in PHP 7.2 and removed in PHP 8.0... But this lib aims to be compatible from PHP 7.1 onward, so... IMHO, this logic needs to be put back.

But I see the bigger optimization about using the offset. Very nice.

Copy link

@thomaslauria thomaslauria Jun 20, 2023

Choose a reason for hiding this comment

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

IMHO, this logic needs to be put back

So do we have to change here something, or shall it remain as it is?

Copy link

@boenrobot boenrobot Jun 20, 2023

Choose a reason for hiding this comment

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

IMHO, this logic needs to be put back

So do we have to change here something, or shall it remain as it is?

It's up to @Shardj , @glensc and I don't know whoever else has permissions to merge.

The above is just my opinion as a fellow user of this library. I am not using mb function overloads in my projects, so I'm not personally affected either way. But if I had merge permissions, I would not merge this without also restoring the function overload logic immediately after.

Choose a reason for hiding this comment

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

restored!

@develart-projects develart-projects added the enhancement New feature or request label Aug 9, 2023
@develart-projects
Copy link
Collaborator

@aleksandar-mitrev I agree with @boenrobot - I would like to merge this one into 1.23, but better to keep overload logic restoration to prevent any possible regression on older projects. Are you able to update the PR, please?

@develart-projects develart-projects added the requested change some changes are needed before the merge label Aug 9, 2023
@develart-projects develart-projects modified the milestones: 1.23.0, 1.24.0 Aug 9, 2023
@develart-projects develart-projects marked this pull request as draft August 23, 2023 12:50
@develart-projects develart-projects removed this from the 1.24.0 milestone Sep 27, 2023
@thomaslauria
Copy link

@develart-projects just have seen that the PR is still open.
Is the latest comment still up to date? So shall we still restore the func_overload stuff?

@develart-projects
Copy link
Collaborator

@thomaslauria yep, still awaiting PR update to get this into the master.

@thomaslauria
Copy link

@develart-projects mbstring stuff restored

@develart-projects develart-projects marked this pull request as ready for review July 22, 2024 12:03
@develart-projects develart-projects removed the requested change some changes are needed before the merge label Jul 22, 2024
@develart-projects develart-projects added this to the 1.24.1 milestone Jul 22, 2024
@develart-projects develart-projects added the to be released PR exists or in master, but not released yet label Jul 22, 2024
@develart-projects develart-projects merged commit be808f8 into Shardj:master Jul 22, 2024
1 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request to be released PR exists or in master, but not released yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants