Skip to content

Reduce memory allocated by var_export, json_encode, serialize, and other #8902

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 8 commits into from
Jul 8, 2022

Conversation

arnaud-lb
Copy link
Member

smart_str uses an over-allocated string to optimize for append operations. Functions that use smart_str tend to return the over-allocated string directly. This results in unnecessary memory usage, especially for small strings.

See #8896

The overhead can be up to 231 bytes for strings smaller than that, and 4095 for other strings.

Here I change a few functions so that they trim the string before returning it.

@devnexen devnexen requested a review from cmb69 June 30, 2022 20:51
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

LGTM! Can't think of a better name for finalize.

@bwoebi
Copy link
Member

bwoebi commented Jun 30, 2022

strpprintf functions also use smart strings. Especially when printing a small value as zend_string, it can be very noticeable. Can that one also be improved?

and 4095 for other strings.

Yes, but this is not solved by a realloc. Allocations larger than 4 KiB are always on a page size multiple.

@devnexen
Copy link
Member

devnexen commented Jul 1, 2022

LGTM! Can't think of a better name for finalize.

Definitively not worth a long debate :-) that s a detail.

@cmb69
Copy link
Member

cmb69 commented Jul 1, 2022

strpprintf functions also use smart strings. Especially when printing a small value as zend_string, it can be very noticeable. Can that one also be improved?

I think so, but that can be done in a separate PR. :)

@devnexen
Copy link
Member

devnexen commented Jul 1, 2022

strpprintf functions also use smart strings. Especially when printing a small value as zend_string, it can be very noticeable. Can that one also be improved?

I think so, but that can be done in a separate PR. :)

I agree on this but worth doing nonetheless.

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

I'm slightly concerned that the additional reallocation might decrease performance. It is okay, if we actually can save memory, but if not (see @bwoebi's comment), it might be better to avoid it.

@bwoebi
Copy link
Member

bwoebi commented Jul 1, 2022

At least if zend_alloc is used, and page_aligned(old_size) == page_aligned(new_size), nothing happens (if > 4 KiB). It's debatable whether it's worth short-circuiting this, as with big strings the relative overhead is minimal.

@arnaud-lb
Copy link
Member Author

arnaud-lb commented Jul 1, 2022

Did some benchmarks to measure the impact on execution time: https://gist.github.com/arnaud-lb/ac223f3e7cf6e6d818adb77bbbd0a182

The first benchmark measures the execution time of var_export($string), with strings of various lengths.

  • There is no impact on strings larger than 4096 bytes
  • When the result of var_export is discarded, there is a negative impact on execution time (17% slower with size=6; 8% with size=12; ). The impact decreases as the size increases.
  • When the result of var_export is accumulated, there is a positive impact on execute time (44% faster with size=6). The impact decreases as the size increases.
  • Short-circuiting the allocator makes no difference

There is no visible difference on other benchmarks (sub-1% difference, results vary in this range from multiple runs)

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Please don't forget to add a note to UPGRADING.INTERNALS when merging. A PR for https://www.phpinternalsbook.com/php7/internal_types/strings/smart_str.html would also be nice.

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.

5 participants