Reduce memory used by UnionType representation. by TysonAndre · Pull Request #729 · phan/phan

TysonAndre · 2017-05-02T07:50:18Z

Performance improvement for phan self-testing (single threaded):

3.68s before this change
3.60s after this change, with php implementation of runkit_object_id()
3.36s (~10% speedup) after this change, with native C implementation of runkit_object_id()

Memory usage reported by PHP decreased by 10%, with or without the native C
implementation.

Helpful if phan is meant to run in daemon mode
Should help users to be able to use more processes to analyze large projects if memory is limited

The UnionType implementation details of the Set are rarely important outside
of a few Phan classes.
Having SplObjectStorage is inefficient for memory usage - An empty
SplObjectStorage requires more memory than an empty array,
and if it's cloned, a clone of the empty SplObjectStorage must be made
In contrast, creating a clone of an array is easy due to copy on write
semantics in php ($x = $y)

This uses an array, with the object id(int) as an array key, and the object itself
as a value.

The only pecl I'm aware of that has the desired functionality
(an integer id for any object, unique for the lifetime of that object)
is runkit (undocumented, but there for a long time).
This has a fork in php7 as well.
This change should work with/without a stripped down module providing
only the function runkit_object_id(object $x) : int
See https://github.com/runkit7/runkit_object_id , which is intended as an optional dependency

Fix bug in suppressing PhanRedefineFunctionInternal

We check if PhanRedefineFunction was suppressed, but should have
checked PhanRedefineFunctionInternal

Add BaseTest, so that static properties can be preserved without being
touched between test runs. Without the special use properties,
PHPUnit would modify the private static properties.

The type of getTypeSet() changed to an array

This contains debugging code, which will be cleaned up later (More interested in the feedback on the change in general)
Also, there are some commented out type assertions (which would pass)

rlerdorf · 2017-05-02T17:46:58Z

Could spl_object_hash() not be used here instead of runkit_object_id()?

TysonAndre · 2017-05-02T19:55:45Z

Could spl_object_hash() not be used here instead of runkit_object_id()?

This would increase overall memory usage, spl_object_hash would be a 32 byte string and would need reference tracking and garbage collection. An int can be stored inline in the PHP's internal array representation, and doesn't need reference counting

Haven't yet checked if that choice would be better or worse than SplObjectHash
There would also be multiple copies of those spl_object_hash strings per object

morria · 2017-05-02T19:35:39Z

+         * @param string|null $type_name
+         * @return UnionType|null
+         */
+        $getForGlobalContext = function($type_name) {


nit: we should use underscores for variable names rather than camel casing

morria · 2017-05-02T19:38:42Z


    /**
     * @param Type[]|\Iterator|null $type_list
+     * @param bool $is_an_array_set - Whether or not this is already a set. Only set to true within UnionSet code.


This is pretty confusing. Why are implementation details leaking out into the public interface?

I didn't think many things would use new UnionType() directly, it's usually helpers from Phan such as UnionType::fromStringInContext, $type->asUnionType(), etc.

morria · 2017-05-02T19:44:42Z


-        return new UnionType(
-            array_map(function (string $type_name) {
+        static $memoizeMap = [];


we should use underscores for variable names.

morria · 2017-05-02T19:45:32Z

-        return new UnionType(
-            array_map(function (string $type_name) {
+        static $memoizeMap = [];
+        $types_set = $memoizeMap[$fully_qualified_string] ?? null;


Do we need to do any normalization on the FQSEN string?

No, this is optional to speed up parsing UnionType a bit (not yet tested)

morria · 2017-05-02T19:46:28Z

+    public static function map(array $object_set, \Closure $cb) : array {
+        $result = [];
+        foreach ($object_set as $object) {
+            $newObject = $cb($object);


we should use underscores for variable names.

morria · 2017-05-02T19:47:55Z


+use ast\Node;
+
+if (!function_exists('runkit_object_id')) {


does Runkit need to marked as a composer dependency? Is it possible to define optional composer dependencies?

https://getcomposer.org/doc/04-schema.md#suggest

runkit_object_id isn't on PECL, so it may be hard to find

Also, this is either of two extensions https://github.com/runkit7/runkit7 or https://github.com/runkit7/runkit_object_id

The suggested option ( https://github.com/runkit7/runkit_object_id/blob/master/runkit_object_id.c ) would be an easier extension to understand.

morria · 2017-05-02T19:48:49Z


+    // Override two magic methods to ensure that Type isn't being cloned accidentally.
+    public function __wakeup() {
+        debug_print_backtrace();


We can probably leave this to be printed by whoever is catching the exception.

rlerdorf · 2017-05-04T15:30:01Z

You can always turn the 32-byte string into an integer. You only need the object handle part which is in the first 16 bytes, so:

hexdec(substr(spl_object_hash($class),0,16));

and you effectively have the object id as an int

TysonAndre · 2017-05-04T19:39:14Z

@rlerdorf this exists already as https://github.com/TysonAndre/phan/blob/62f988bd726506a1c1d07714a5b53649682a0fc2/src/runkit.php

rlerdorf · 2017-05-04T19:45:27Z

Surprised the runkit version is so much faster than that wrapper. Sounds like we should add spl_object_id() to PHP then because having a dependency on runkit is terrible, even if it is optional.

TysonAndre · 2017-05-14T00:23:44Z

Sounds like we should add spl_object_id() to PHP then because having a dependency on runkit is terrible, even if it is optional.

EDIT: Found an identical proposal at https://marc.info/?t=143835274500003&r=1&w=2
EDIT: should be http://grokbase.com/t/php/php-internals/08chfwdavh/new-function-proposal-spl-object-id#2008121730trg75pyz8mn92dqwemjb14k8 (I assume the object id should be unique even if the pointer to object handler table can differ between object instances)

So if I XOR it with the same mt_srand() as spl_object_hash would for those bytes (representing object id), it's possibly acceptable?

https://bugs.php.net/bug.php?id=52657

It's been requested for a while, but they hope to be able to move away from keeping object ids if they need to (The exact object id is currently also available inside a larger string in var_dump.

And they want "globally unique" instead of "unique for the lifetime of the object" (runkit_object_id and spl_object_hash are the latter)

Also, spl_object_hash returns it xored() with a random number, to make php slightly harder to exploit in some cases I'm not familiar with (runkit_object_id extension could add that if needed))

It's not really a surprise pure PHP would be slow, since the wrapper would be called frequently in the proposed use case, and allocates a string, converts numbers to a string, allocates a substring of that string (and calls a function to do that), then converts the string back into numbers.

…presentation Add a common base test class, to make common configuration easier in future PRs.

…presentation And add test of ArrayAccess implementation, to avoid regression. Add a common base test class, to make common configuration easier in future PRs.

Add related code changes from #729, without changing UnionType representation

TysonAndre · 2017-06-21T15:51:34Z

Fixed merge conflicts. Converted some relative function calls to fully qualified function calls.

Performance improvement: - 3.68s before this change - 3.60s after this change, with php implementation of runkit_object_id() - 3.36s after this change, with native C implementation of runkit_object_id() Memory usage decreased by 10%, with or without the native C implementation. The UnionType implementation details of the Set are rarely important outside of a few Phan classes. Having SplObjectStorage is inefficient for memory usage - An empty SplObjectStorage requires more memory than an empty array, and if it's cloned, a clone of the empty SplObjectStorage must be made In contrast, creating a clone of an array is easy due to copy on write semantics in php (`$x = $y`) This uses an array, with the object id as an array key, and the object itself as a value. - The only pecl I'm aware of that has the desired functionality (an integer id for any object, unique for the lifetime of that object) is runkit (undocumented, but there for a long time). This has a fork in php7 as well. This change should work with/without a stripped down module providing only the function `runkit_object_id(object $x) : int` See https://github.com/runkit7/runkit_object_id Fix bug in suppressing PhanRedefineFunctionInternal - We check if PhanRedefineFunction was suppressed, but should have checked PhanRedefineFunctionInternal Add BaseTest, so that static properties can be preserved without being touched between test runs. Without the special use properties, PHPUnit would modify the private static properties. Fix loading the fallback implementation of runkit_object_id, fix test The first place the fallback is used is in Type::make(), so require_once the fallback in Type.php (Need to load fallback when running and in unit tests) Also, the Type::make() object caching logic changed, causing object to be confused with \\object, etc. - Use a different key space for NativeType by adding a unique prefix. (Only shows up in the factory method) Address review comments. Use underscores for variable names. Update name in phpdoc Use fully qualified function names to speed up calls to those functions.

TysonAndre · 2017-06-23T06:25:55Z

Surprised the runkit version is so much faster than that wrapper. Sounds like we should add spl_object_id() to PHP then because having a dependency on runkit is terrible, even if it is optional.

The spl_object_id with the obfuscated address has a POC at TysonAndre/php-src#1 (but it assumes sizeof(zend_long) >= sizeof(intptr_t), and doesn't have an RFC.

TysonAndre requested a review from morria May 2, 2017 07:50

TysonAndre force-pushed the optimize-uniontype-memory-upstream branch from 50a97dc to cab026c Compare May 2, 2017 07:52

morria reviewed May 4, 2017

View reviewed changes

TysonAndre force-pushed the optimize-uniontype-memory-upstream branch 2 times, most recently from 3c345fc to 962a7fa Compare May 14, 2017 00:04

TysonAndre force-pushed the optimize-uniontype-memory-upstream branch from 9bd7684 to 6b0364a Compare June 1, 2017 04:21

TysonAndre added a commit to TysonAndre/phan that referenced this pull request Jun 1, 2017

Add related code changes from phan#729, without changing UnionType re…

34e58f2

…presentation Add a common base test class, to make common configuration easier in future PRs.

TysonAndre mentioned this pull request Jun 1, 2017

Add related code changes from #729, without changing UnionType representation #801

Merged

TysonAndre added a commit that referenced this pull request Jun 1, 2017

Merge pull request #801 from TysonAndre/related-code-cleanup-optimize

12d3694

Add related code changes from #729, without changing UnionType representation

TysonAndre force-pushed the optimize-uniontype-memory-upstream branch from 6b0364a to 83c1098 Compare June 1, 2017 04:46

TysonAndre mentioned this pull request Jun 7, 2017

Add scripts or command line flag to check if phan is running with optimal performance #843

Open

TysonAndre added the experimental change This is being considered by Phan's maintainers label Jun 11, 2017

TysonAndre force-pushed the optimize-uniontype-memory-upstream branch from 83c1098 to c5e43f8 Compare June 12, 2017 05:25

TysonAndre force-pushed the optimize-uniontype-memory-upstream branch from c5e43f8 to a992e25 Compare June 21, 2017 15:50

TysonAndre added 2 commits June 22, 2017 22:47

whitespace nit

8e12624

TysonAndre force-pushed the optimize-uniontype-memory-upstream branch from a992e25 to 8e12624 Compare June 23, 2017 05:49

update NEWS

aecaa84

TysonAndre added 2 commits June 22, 2017 23:33

use fully qualified function call for commonly used uniontype methods

4d8507a

speed up context merging

a1469e2

TysonAndre merged commit 5cac61e into phan:master Jun 23, 2017

TysonAndre deleted the optimize-uniontype-memory-upstream branch June 25, 2017 02:47

TysonAndre removed the experimental change This is being considered by Phan's maintainers label Jun 22, 2018

Uh oh!

Conversation

TysonAndre commented May 2, 2017

Uh oh!

rlerdorf commented May 2, 2017

Uh oh!

TysonAndre commented May 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TysonAndre May 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rlerdorf commented May 4, 2017

Uh oh!

TysonAndre commented May 4, 2017

Uh oh!

rlerdorf commented May 4, 2017

Uh oh!

TysonAndre commented May 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TysonAndre commented Jun 21, 2017

Uh oh!

TysonAndre commented Jun 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TysonAndre May 14, 2017 •

edited

Loading

TysonAndre commented May 14, 2017 •

edited

Loading