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

Fixes property parsing being invoked inefficiently #132

Merged
merged 3 commits into from Sep 8, 2018

Conversation

Projects
None yet
2 participants
@dakujem
Contributor

dakujem commented Sep 5, 2018

The problem:

  • property parsing was being triggered multiple times for the same files, exponentially in relation to the ancestry depth

The solution:

  • do not call property parsing in the constructor
  • parse properties on first access

The result:

  • the complexity of parsing is reduced from exponential complexity to linear complexity, in relation to the depth of ancestry of the final entity classes

The test that shows the problem can be seen here:
https://github.com/dakujem/LeanMapper/blob/props-parsing-hell/tests/LeanMapper/EntityReflection.parsing.phpt

The tests for the fixed version can be found here:
https://github.com/dakujem/LeanMapper/blob/props-parsing-test/tests/LeanMapper/EntityReflection.parsing.phpt

I did not include the test intentionally, as it requires a static hook in the EntityReflection class or changing the visibility of methods in the class (private->protected). I am open to discussion.

@janpecha

This comment has been minimized.

Show comment
Hide comment
@janpecha

janpecha Sep 5, 2018

Collaborator

Cool, thanks! I'll look at it tomorrow. Only one note - it breaks some tests, can you fix them? Just change

$reflection = Abc::getReflection()

to

$reflection = Abc::getReflection();
$reflection->getEntityProperties();

Thanks!

Collaborator

janpecha commented Sep 5, 2018

Cool, thanks! I'll look at it tomorrow. Only one note - it breaks some tests, can you fix them? Just change

$reflection = Abc::getReflection()

to

$reflection = Abc::getReflection();
$reflection->getEntityProperties();

Thanks!

@dakujem

This comment has been minimized.

Show comment
Hide comment
@dakujem

dakujem Sep 6, 2018

Contributor

I see. Now that the props are lazy loaded, the constructor does not trigger the errors. I will update the tests.

Anyway, what is your configuration to run th tests? I did not find any info on running tests, or contributing in general.

Contributor

dakujem commented Sep 6, 2018

I see. Now that the props are lazy loaded, the constructor does not trigger the errors. I will update the tests.

Anyway, what is your configuration to run th tests? I did not find any info on running tests, or contributing in general.

@janpecha

This comment has been minimized.

Show comment
Hide comment
@janpecha

janpecha Sep 6, 2018

Collaborator

I use vendor/bin/tester -p php -c tests/php-unix.ini tests/ (on Ubuntu 16.04 with PHP 7.2).

  1. composer install
  2. edit tests/php-unix.ini (if needed)
  3. vendor/bin/tester -p php -c tests/php-unix.ini tests/
Collaborator

janpecha commented Sep 6, 2018

I use vendor/bin/tester -p php -c tests/php-unix.ini tests/ (on Ubuntu 16.04 with PHP 7.2).

  1. composer install
  2. edit tests/php-unix.ini (if needed)
  3. vendor/bin/tester -p php -c tests/php-unix.ini tests/
@dakujem

This comment has been minimized.

Show comment
Hide comment
@dakujem

dakujem Sep 6, 2018

Contributor

I updated the failing test. I also added another optimization (I forgot to include it yesterday) that enables on-demand getter/setter initialization. The performance impact of it is not so remarkable, but removes some unncessary method name parsings.

Contributor

dakujem commented Sep 6, 2018

I updated the failing test. I also added another optimization (I forgot to include it yesterday) that enables on-demand getter/setter initialization. The performance impact of it is not so remarkable, but removes some unncessary method name parsings.

@dakujem

This comment has been minimized.

Show comment
Hide comment
@dakujem

dakujem Sep 6, 2018

Contributor

By the way, the tests fail using PHP 7.2.4:

-- FAILED: Entity.emptyQuery.phpt
   Exited with error code 255 (expected 0)
   E_WARNING: count(): Parameter must be an array or an object that implements Countable

   in tests\LeanMapper\Entity.emptyQuery.phpt(38)

-- FAILED: Entity.assign.1.phpt
   Exited with error code 255 (expected 0)
   E_DEPRECATED: The each() function is deprecated. This message will be suppressed on further calls

   in tests\LeanMapper\Entity.assign.1.phpt(77)
   in tests\LeanMapper\Entity.assign.1.phpt(77) each()
Contributor

dakujem commented Sep 6, 2018

By the way, the tests fail using PHP 7.2.4:

-- FAILED: Entity.emptyQuery.phpt
   Exited with error code 255 (expected 0)
   E_WARNING: count(): Parameter must be an array or an object that implements Countable

   in tests\LeanMapper\Entity.emptyQuery.phpt(38)

-- FAILED: Entity.assign.1.phpt
   Exited with error code 255 (expected 0)
   E_DEPRECATED: The each() function is deprecated. This message will be suppressed on further calls

   in tests\LeanMapper\Entity.assign.1.phpt(77)
   in tests\LeanMapper\Entity.assign.1.phpt(77) each()
@dakujem

This comment has been minimized.

Show comment
Hide comment
@dakujem

dakujem Sep 6, 2018

Contributor

vendor/bin/tester -p php -c tests/php-unix.ini tests/ does not really work, because at least sqlite is needed. I'm running the tests in my local environment using tester v2 an the switch -C (using my local cli configuration). I suggest using this configuration, contributing will be easier for general public.

I'm also using this snippet in composer.json:

	"scripts": {
		"test":"tester tests -C"
	}

then it is very easy to just type composer test and have the tests running.

Contributor

dakujem commented Sep 6, 2018

vendor/bin/tester -p php -c tests/php-unix.ini tests/ does not really work, because at least sqlite is needed. I'm running the tests in my local environment using tester v2 an the switch -C (using my local cli configuration). I suggest using this configuration, contributing will be easier for general public.

I'm also using this snippet in composer.json:

	"scripts": {
		"test":"tester tests -C"
	}

then it is very easy to just type composer test and have the tests running.

@janpecha

This comment has been minimized.

Show comment
Hide comment
@janpecha

janpecha Sep 6, 2018

Collaborator

I updated the failing test.

Please also update Entity.hasMany.inversed.phpt (see https://travis-ci.org/Tharos/LeanMapper/builds/425134127)

on-demand getter/setter initialization

You read my mind :)

...tests fail using PHP 7.2.4

Have you latest version? It was fixed in 62a7363.

because at least sqlite is needed.

Yeah, the tests currently require json, pdo, sqlite3 and tokenizer extensions (on my configuration). They are listed in php-unix.ini.

Collaborator

janpecha commented Sep 6, 2018

I updated the failing test.

Please also update Entity.hasMany.inversed.phpt (see https://travis-ci.org/Tharos/LeanMapper/builds/425134127)

on-demand getter/setter initialization

You read my mind :)

...tests fail using PHP 7.2.4

Have you latest version? It was fixed in 62a7363.

because at least sqlite is needed.

Yeah, the tests currently require json, pdo, sqlite3 and tokenizer extensions (on my configuration). They are listed in php-unix.ini.

dakujem added some commits Sep 5, 2018

Fixes property parsing being invoked inefficiently
- property parsing was being triggered multiple times for the same files, exponentially in relation to the ancestry depth
@dakujem

This comment has been minimized.

Show comment
Hide comment
@dakujem

dakujem Sep 7, 2018

Contributor

Sorry, guys. I did not realize that I had forked the repo before 🤣 I was thinking I was working on the latest version, because I clicked the fork button on the day I created the PR...
Anyway, I synced the repo and all the tests are passing now. Even those 7.2 issues disappeared, as you mentioned.

@janpecha I believe the PR is now ready to be merged.

Contributor

dakujem commented Sep 7, 2018

Sorry, guys. I did not realize that I had forked the repo before 🤣 I was thinking I was working on the latest version, because I clicked the fork button on the day I created the PR...
Anyway, I synced the repo and all the tests are passing now. Even those 7.2 issues disappeared, as you mentioned.

@janpecha I believe the PR is now ready to be merged.

@janpecha

This comment has been minimized.

Show comment
Hide comment
@janpecha

janpecha Sep 7, 2018

Collaborator

@dakujem Can you squash commits into single commit with message (for example):

EntityReflection: improved performance

Added on-demand property parsing and getter/setter initialization.

Thanks!

Collaborator

janpecha commented Sep 7, 2018

@dakujem Can you squash commits into single commit with message (for example):

EntityReflection: improved performance

Added on-demand property parsing and getter/setter initialization.

Thanks!

@dakujem

This comment has been minimized.

Show comment
Hide comment
@dakujem

dakujem Sep 7, 2018

Contributor

Hey, I thought GitHub offers this feature when merging. If not, let me know and I will squash.

Contributor

dakujem commented Sep 7, 2018

Hey, I thought GitHub offers this feature when merging. If not, let me know and I will squash.

@janpecha janpecha merged commit f3b7e3d into Tharos:develop Sep 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@janpecha

This comment has been minimized.

Show comment
Hide comment
@janpecha

janpecha Sep 8, 2018

Collaborator

Yes, Github offers this feature, but from my point of view is better if author of PR prepare it because final commit is tested on CI once again before merging.

Merged, thanks!

Collaborator

janpecha commented Sep 8, 2018

Yes, Github offers this feature, but from my point of view is better if author of PR prepare it because final commit is tested on CI once again before merging.

Merged, thanks!

@dakujem

This comment has been minimized.

Show comment
Hide comment
@dakujem

dakujem Sep 8, 2018

Contributor

I will do that next time.

Contributor

dakujem commented Sep 8, 2018

I will do that next time.

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