Skip to content

feat: replace proxy mechanism with PHP 8.4 lazy objects #893

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

Open
wants to merge 2 commits into
base: 2.6.x
Choose a base branch
from

Conversation

nikophil
Copy link
Member

@nikophil nikophil commented May 5, 2025

relates to #899

I think we'll release this in Foundry 2.6

@nikophil nikophil marked this pull request as draft May 5, 2025 11:36
@nikophil nikophil force-pushed the feat/proxy-php84 branch from 373d1e4 to ecdb926 Compare May 5, 2025 11:51
@nikophil nikophil force-pushed the feat/proxy-php84 branch from ecdb926 to 76df287 Compare May 5, 2025 11:56
Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

When called without args, let's make the refresh() function proxify everything.

@nikophil
Copy link
Member Author

nikophil commented May 5, 2025

When called without args, let's make the refresh() function proxify everything.

this feels weird, refresh() function always returns something:

function refresh(object &$object): object
{
    return Configuration::instance()->persistence()->refresh($object);
}

maybe we could add a new function?

@kbond
Copy link
Member

kbond commented May 5, 2025

this feels weird, refresh() function always returns something:

Hmm yeah, forgot about that. maybe a refresh_all()

@nikophil nikophil force-pushed the feat/proxy-php84 branch 2 times, most recently from 9bd9a76 to 6fedc74 Compare May 6, 2025 07:12
@nikophil
Copy link
Member Author

nikophil commented May 6, 2025

refresh_all() done 👍

@nikophil nikophil force-pushed the feat/proxy-php84 branch 7 times, most recently from 57b3f02 to 4c21aaf Compare May 9, 2025 06:30
@nikophil
Copy link
Member Author

nikophil commented May 9, 2025

OK CI is now green 🙂

I've deprecated PersistentProxyObjectFactory for PHP 8.4 and handled the deprecations in our testsuite

I've listed in an issue what needs to be done before releasing this

Benchmark is showing a small performance downgrade, which seems totally legit, because we're performing more requests than before with PersistentObjectFactory, but I'm pretty sure that "new Proxy 8.4 Factory" VS the old PersistentProxyObjectFactory (which basically everybody uses) is equivalent in performance, or maybe better

+--------------------------------+-------------------+-----+------+-----+-----------------+------------------+----------------+
| benchmark                      | subject           | set | revs | its | mem_peak        | mode             | rstdev         |
+--------------------------------+-------------------+-----+------+-----+-----------------+------------------+----------------+
| ContactFactoryBench            | bench_create      |     | 10   | 5   | 17.728mb +0.24% | 2.038ms +0.61%   | ±5.23% +32.84% |
| ContactFactoryBench            | bench_create_many | 1   | 10   | 5   | 17.730mb +0.24% | 2.185ms +7.07%   | ±3.36% +45.21% |
| ContactFactoryBench            | bench_create_many | 10  | 10   | 5   | 18.610mb +0.49% | 10.736ms +2.39%  | ±1.80% +32.57% |
| ContactFactoryBench            | bench_create_many | 50  | 10   | 5   | 22.730mb +1.06% | 46.839ms -0.83%  | ±1.03% +1.36%  |
| CategoryFactoryBench           | bench_create      |     | 10   | 5   | 17.945mb +0.33% | 5.206ms +6.12%   | ±1.53% -13.05% |
| CategoryFactoryBench           | bench_create_many | 1   | 10   | 5   | 17.957mb +0.33% | 5.490ms +2.20%   | ±3.03% -15.99% |
| CategoryFactoryBench           | bench_create_many | 10  | 10   | 5   | 21.388mb +1.03% | 38.583ms +0.33%  | ±0.67% -46.50% |
| CategoryFactoryBench           | bench_create_many | 50  | 10   | 5   | 36.158mb +2.20% | 182.886ms -0.06% | ±0.47% -49.97% |
| PersistentDocumentFactoryBench | bench_create      |     | 10   | 5   | 16.240mb +0.24% | 746.168μs -1.00% | ±2.78% +83.50% |
| PersistentDocumentFactoryBench | bench_create_many | 1   | 10   | 5   | 16.245mb +0.24% | 774.627μs -0.07% | ±1.48% -11.57% |
| PersistentDocumentFactoryBench | bench_create_many | 10  | 10   | 5   | 16.310mb +0.74% | 4.824ms -1.42%   | ±4.77% +13.25% |
| PersistentDocumentFactoryBench | bench_create_many | 50  | 10   | 5   | 17.943mb +1.07% | 22.993ms +0.52%  | ±1.01% -45.65% |
+--------------------------------+-------------------+-----+------+-----+-----------------+------------------+----------------+

@nikophil nikophil force-pushed the feat/proxy-php84 branch from bc699b8 to e48de62 Compare May 9, 2025 07:33
@nikophil nikophil force-pushed the feat/proxy-php84 branch from e48de62 to 53b2360 Compare May 9, 2025 07:41
@nikophil nikophil force-pushed the feat/proxy-php84 branch from 53b2360 to 741d1e6 Compare May 9, 2025 08:01
@nikophil nikophil requested a review from kbond May 9, 2025 08:05
@nikophil nikophil marked this pull request as ready for review May 9, 2025 08:07
@nikophil nikophil force-pushed the feat/proxy-php84 branch 2 times, most recently from 43bd959 to 8a01c56 Compare May 9, 2025 08:41
@nikophil nikophil changed the title feat: create proxy system with PHP 8.4 lazy proxies feat: replace proxy mechanism with PHP 8.4 lazy proxies May 9, 2025
@nikophil nikophil changed the title feat: replace proxy mechanism with PHP 8.4 lazy proxies feat: replace proxy mechanism with PHP 8.4 lazy objects May 9, 2025
Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

Looking good! Thanks for working on this!

@nikophil nikophil force-pushed the feat/proxy-php84 branch from 8a01c56 to f16f893 Compare May 12, 2025 08:25
@nikophil nikophil force-pushed the 2.5.x branch 3 times, most recently from 690b95a to 75dd015 Compare May 12, 2025 16:43
@nikophil nikophil force-pushed the feat/proxy-php84 branch from f16f893 to 5c886d2 Compare May 12, 2025 17:10
@nikophil nikophil changed the base branch from 2.5.x to 2.6.x May 12, 2025 17:11
@nikophil nikophil force-pushed the feat/proxy-php84 branch 3 times, most recently from 21860d7 to 067851e Compare May 18, 2025 19:51
@nikophil nikophil force-pushed the feat/proxy-php84 branch from 067851e to 58dbe05 Compare May 18, 2025 20:05
@nikophil
Copy link
Member Author

@kbond I think this PR is ready - still needs some work to be done (see #899), but in another PR

@nikophil nikophil requested a review from kbond May 18, 2025 20:16
Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

Looking good! Think we should release 2.6 before merging?

@@ -176,6 +201,24 @@ function enable_persisting(): void
Configuration::instance()->persistence()->enablePersisting();
}

function assertPersisted(object $object, string $message = '{entity} is not persisted.'): void
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we should stick with our current snake_case function naming.

In a future PR, thinking we should add some helper phpunit methods to either Factories or a new trait:

  • ->save()
  • ->assertPersisted()
  • etc...

Copy link
Member Author

@nikophil nikophil May 22, 2025

Choose a reason for hiding this comment

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

I feel like we should stick with our current snake_case function naming.

do you mean I should change to _assertPersisted()?

In a future PR, thinking we should add some helper phpunit methods to either Factories or a new trait

yeah, why not

Copy link
Member

Choose a reason for hiding this comment

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

do you mean I should change to _assertPersisted()?

assert_persisted() to match the other functions like persistent_factory()

Copy link
Member Author

Choose a reason for hiding this comment

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

LOL ok sorry 😅 🤦

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants