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

Make inspecting SplFixedArray instances more memory efficient/consistent, change print_r null props handling #9757

Merged
merged 2 commits into from
Oct 24, 2022

Conversation

TysonAndre
Copy link
Contributor

  • Print objects with null hash table like others in print_r

    Noticed when working on subsequent commits for SplFixedArray.
    Make whether zend_get_properties_for returns null or an empty array
    invisible to the end user - it would be always be a non-null array for
    user-defined classes.

  • Make handling of SplFixedArray properties more consistent

    Create a brand new reference counted array every time in SplFixedArray
    to be freed by the callers (or return null).

    There were various inconsistencies in SplFixedArray that this fixes, (which required improvements only available in 8.3 from PRs I merged):

    • Calling var_export/var_dump/array cast/print_r on an SplFixedArray directly/indirectly would increase its memory usage by allocating and populating its property table
    • Calling destructors on values found in an SplFixedArray would be delayed if those properties were found in the most recent call to var_export/var_dump/etc
    • The properties table does not need to be created (increasing memory usage) then returned to get_gc if there are no dynamic object properties (similar to the implementation of zend_std_get_gc)

"object properties" refers to $splFixedArray->prop
"array elements" refers to $splFixedArray[$offset]

This approach is similar to the one used for final class Collections\Deque, but also needs to account for the fact that SplFixedArray allows dynamic properties, can be subclassed, and those subclasses will be able to have #[AllowDynamicProperties] even in php 9.0. #7500

Also see https://github.com/php/php-src/pull/9703/files (PRs have no dependencies can be merged independently if approved)

For datastructures such as SplFixedArray, the call to get_properties currently also takes time and may have side effects(destructors), because it needs to update/delete the values to match the backing array. Maybe SplFixedArray and similar datastructures should switch to get_properties_for, so that get_mangled_object_props returns only properties and not fields


References

This is unrelated to 9697, but I wrote up what seems to be the requirements for properly implementing get_properties and get_properties_for handlers that will work correctly with the various internal functionality of php-src in #9697 (comment) . This was used when working on https://github.com/TysonAndre/pecl-teds/ and its unit tests, as well as the Deque RFC implementation #7500

@TysonAndre
Copy link
Contributor Author

Test failures seem spurious and unrelated

ext/pdo_oci/tests/oci_success_with_info.phpt (github, click to expand)
       [2]=>
       string(%d) "OCISessionBegin: OCI_SUCCESS_WITH_INFO: ORA-28002: %s
      (%s:%d)"
010+ 
011+ Warning: PDO::exec(): SQLSTATE[HY000]: General error: 1940 OCIStmtExecute: ORA-01940: cannot drop a user that is currently connected
012+ ORA-06512: at line 6
013+ ORA-06512: at line 2
014+  (/home/runner/work/php-src/php-src/ext/pdo_oci/oci_driver.c:342) in /home/runner/work/php-src/php-src/ext/pdo_oci/tests/oci_success_with_info.php on line 26
     array(3) {
       [0]=>
       string(5) "HY000"
sapi/fpm/tests/status-listen.phpt (travis, click to expand) ========DIFF======== 001+ ERROR: Expected body does not match pattern 001- Done 002+ BODY: 003+ string(377) "pool: unconfined 004+ process manager: static 005+ start time: 15/Oct/2022:20:20:12 +0000 006+ start since: 0 007+ accepted conn: 1 008+ listen queue: 0 009+ max listen queue: 0 010+ listen queue len: 4096 011+ idle processes: 0 012+ active processes: 1 013+ total processes: 1 014+ max active processes: 1 015+ max children reached: 0 016+ slow requests: 0" 017+ PATTERN: 018+ string(345) "(pool:\s+unconfined 019+ process manager:\s+static 020+ start time:\s+\d+\/\w{3}\/\d{4}:\d{2}:\d{2}:\d{2}\s[+-]\d{4} 021+ start since:\s+\d+ 022+ accepted conn:\s+\d+ 023+ listen queue:\s+0 024+ max listen queue:\s+0 025+ listen queue len:\s+\d+ 026+ idle processes:\s+1 027+ active processes:\s+0 028+ total processes:\s+1 029+ max active processes:\s+1 030+ max children reached:\s+0 031+ slow requests:\s+0)" 032+ Done ========DONE======== FAIL FPM: Status listen test [sapi/fpm/tests/status-listen.phpt]

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.

SPL changes LGTM, and I don't really care about the changes to print_r but some might disagree.

Create a brand new reference counted array every time in SplFixedArray
to be freed by the callers (or return null).
Noticed when working on subsequent commits for SplFixedArray.
Make whether zend_get_properties_for returns null or an empty array
invisible to the end user - it would be always be a non-null array for
user-defined classes.

Noticed when working on SplFixedArray changes, e.g. in
ext/spl/tests/SplFixedArray__construct_param_null.phpt
@TysonAndre TysonAndre force-pushed the printing-consistency branch from 9a347ee to d726002 Compare October 23, 2022 15:00
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

I don't see problems.

@@ -5,15 +5,15 @@ Objects with overloaded get_properties are incompatible with ArrayObject

$ao = new ArrayObject([1, 2, 3]);
try {
$ao->exchangeArray(new SplFixedArray);
$ao->exchangeArray(new DateInterval('P1D'));
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Overloaded object of type DateInterval is not compatible with ArrayObject

ArrayObject checks if get_properties_handler is overloaded and throws if it is.

After my change to spl_fixedarray.c, the get_properties handler of SplFixedArray is no longer overloaded.

So I changed to DateInterval, which continues to overload the get_properties handler

@TysonAndre
Copy link
Contributor Author

Build failures are unrelated.

1ea8c82 fixed the unrelated spurious travis build failure https://ci.appveyor.com/project/php/php-src/builds/45155001/job/767ocx20lg4y70vg

the spurious php-fpm test failure is a known issue https://app.travis-ci.com/github/php/php-src/jobs/586419479

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