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

Optimize ReplaceAliasByActualDefinitionPass #18265

Merged
merged 1 commit into from Mar 31, 2016

Conversation

Projects
None yet
@ajb-in
Copy link
Contributor

commented Mar 22, 2016

Q A
Branch? 2.3
Bug fix? yes (performance)
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Previous implementation passed over every definition for every alias (n*m runtime).
New implementation passes once over all aliases and once over all definitions (n+m).
Also removing needless "restart" logic.

@ajb-in

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2016

Note: patch applies to master with minor modification (tested).

@lstrojny

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2016

Improves container dump times from ~40s to ~16s in real world applications.

}
return $arguments;
}
private function updateFactoryServiceReference($factoryService, $currentId, $newId)
private function updateFactoryReferenceId($replacements, $referenceId)

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Mar 22, 2016

Contributor

What about adding a doc block here?

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

On it 👍

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

Done

@@ -39,98 +38,74 @@ public function process(ContainerBuilder $container)
$this->compiler = $container->getCompiler();
$this->formatter = $this->compiler->getLoggingFormatter();
foreach ($container->getAliases() as $id => $alias) {
$seenAliases = array();

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Mar 22, 2016

Contributor

IMO $treatedAliases or $managedAliases would be better.

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

This keeps tracks of all aliases seen in this pass, whether they need replacement (i.e. point to a private def) or not.

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Mar 22, 2016

Member

$processedAliases ? In any case, I agree with @Ener-Getick: $seenAliases seems wrong because of the seen verb.

This comment has been minimized.

Copy link
@stof

stof Mar 22, 2016

Member

This does not keep track of aliases you saw in the past, but of alias targets you saw in the past (which might not be aliases at all)

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

👍 renaming to seenAliasTargets

try {
$definition = $container->getDefinition($aliasId);
} catch (InvalidArgumentException $e) {
throw new InvalidArgumentException(sprintf('Unable to replace alias "%s" with actual definition "%s".', $id, $alias), null, $e);
$msg = sprintf('Unable to replace alias "%s" with actual definition "%s".', $defId, $alias);
throw new InvalidArgumentException($msg, null, $e);

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Mar 22, 2016

Contributor

should be inlined.

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

👌

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

Done

}
$refId = (string) $argument;
if (!array_key_exists($refId, $replacements)) {

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Mar 22, 2016

Contributor

isset is faster

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

Can I safely assume it'll never be null?

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Mar 22, 2016

Contributor

This looks strange to me that a service id could be null but array_key_exists is used in ContainerBuilder so I don't know.

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

Makes sense. Changed it to isset

This comment has been minimized.

Copy link
@stof

stof Mar 22, 2016

Member

A service id is always a string

foreach ($container->getAliases() as $id => $alias) {
$seenAliases = array();
$replacements = array();
foreach ($container->getAliases() as $defId => $alias) {

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Mar 22, 2016

Contributor

IMO $id was fine.

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

I found it ambiguous since that section handles several ids simultaneously.

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Mar 22, 2016

Member

Short variable names usually reduce legibility: what about using $definitionId?

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

👍

$aliasId = (string) $alias;
if (array_key_exists($aliasId, $replacements)) {
$container->setAlias($defId, $replacements[$aliasId]);

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Mar 22, 2016

Contributor

The goal is to remove the initial service when possible so this should be removed.

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

Actually I think that's equivalent to what the original code was doing: all subsequent aliases that pointed to the old (removed) alias are changed to point to the new one.

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Mar 22, 2016

Contributor

Only private services are replaced so they don't need to be available after this optimization.

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

So, should references to these aliases also be replaced in definitions?

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Mar 22, 2016

Contributor

Ah sorry you're right, I thought it was doing something else...

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

So I should leave it that way then?

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Mar 22, 2016

Contributor

Yeah that's fine 👍

@ajb-in ajb-in force-pushed the ajb-in:2.3 branch 2 times, most recently from 0f77f69 to 4783143 Mar 22, 2016

$arguments[$k] = $this->updateArgumentReferences($replacements, $defId, $argument);
continue;
}
if (!($argument instanceof Reference)) {

This comment has been minimized.

Copy link
@stloyd

stloyd Mar 22, 2016

Contributor

Should be:

if (!$argument instanceof Reference)

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

That looks as though it said (!$argument) instanceof Reference). I know precedence rules work the other way around, but it still looks visually confusing IMO. I'll change it if it's a big fugly though.

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

Done :(

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Mar 22, 2016

Contributor

@ajb-in this is always written this way in symfony (see this)

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

Fair enough 🍺

@GuilhemN

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2016

This looks great 👍
Thank you @ajb-in 😄

@ajb-in ajb-in force-pushed the ajb-in:2.3 branch from 4783143 to 5e5cdd0 Mar 22, 2016

@ajb-in

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2016

Thanks @Ener-Getick ^_^

$aliasId = (string) $alias;
if (isset($replacements[$aliasId])) {
$container->setAlias($defId, $replacements[$aliasId]);

This comment has been minimized.

Copy link
@stof

stof Mar 22, 2016

Member

This must respect the visibility of the existing alias (allowing it to be removed in case of a private alias not being referenced anywhere anymore after other optimizations)

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

I tried to stick to the original behaviour, namely: remove the definition of the target, point the alias to the definition, and point every subsequent alias with the same target to the first alias.

}
$refId = (string) $argument;
if (!isset($replacements[$refId])) {

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Mar 22, 2016

Member

Same here: what about using $referenceId?

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

👍

}

This comment has been minimized.

Copy link
@stof

stof Mar 22, 2016

Member

please don't remove empty lines. there are here for readability of the source code

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

Can we compromise on section-breaking comments? (see new commit) 😄

$aliasId = (string) $alias;
if (isset($replacements[$aliasId])) {

This comment has been minimized.

Copy link
@stof

stof Mar 22, 2016

Member

What if you have chained aliased, and the target alias is processed only later ? This would not replace it properly anymore (which makes me think that this case is not properly covered by tests btw)

This comment has been minimized.

Copy link
@stof

stof Mar 22, 2016

Member

the easiest way to handle it would probably be to restart the handling of aliases to build the table of replacements, keeping the update of Definitions for the end to do it only once.

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

Restarting the pass would mean quadratic runtime, which would beat the purpose of this patch.
I could do it with a union-find structure though. Could I get a test case to check against please? :3

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

@stof: so the logic should be: if I have a chain a1 -> a2 -> a3 -> a4 -> d (with d private), that should change to a1 -> a3, a2 -> a3, a3 -> d (with d public) and a4 deleted?

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

@stof: also, should alias chains that end in a public definition be affected?

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 23, 2016

Author Contributor

@stof: Looking at the original code again, I think the restart step doesn't do anything anyway. Aliases are replaced only if the definition of their target is marked as private; the process is to mark the definition as public and replace all references to the alias target with references to the alias id (and then removing the definition of the alias target). This process does not introduce any new private definitions, therefore no alias target that was previously seen as having a public definition should have a private one after it. Am I missing something?

{
if (null === $factoryService) {

This comment has been minimized.

Copy link
@stof

stof Mar 22, 2016

Member

This case must be kept, to avoid issues in case there is a service with an empty string id (which is valid)

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

Wouldn't that case be handled normally by the logic?

This comment has been minimized.

Copy link
@stof

stof Mar 22, 2016

Member

no, because tried to access $replacements[$referenceId] will access $replacements[''], as null is not a valid array key and so get casted string

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

Oh fun 😒
Should I be explicit and return null; here or leave it as return?

This comment has been minimized.

Copy link
@stof

stof Mar 22, 2016

Member

leave it as it is today, due to the Symfony coding standards (even though I personally don't like this particular rule)

This comment has been minimized.

Copy link
@SenseException

SenseException Mar 23, 2016

Contributor

@stof Why was an empty string defined as valid?

This comment has been minimized.

Copy link
@stof

stof Mar 23, 2016

Member

@SenseException there is nothing to do to define a string as valid. there is work to do to forbid it. And given that we have not written code to forbid it, it means that the empty id is accepted by the code today, and so must continue to work (in case someone had the weird idea to use it).

$arguments[$k] = $this->updateArgumentReferences($replacements, $defId, $argument);
continue;
}
if (!$argument instanceof Reference) {

This comment has been minimized.

Copy link
@stof

stof Mar 22, 2016

Member

please add empty lines between the different if blocks for readability

This comment has been minimized.

Copy link
@ajb-in

ajb-in Mar 22, 2016

Author Contributor

better?

@ajb-in ajb-in force-pushed the ajb-in:2.3 branch from 9e52734 to 83f87da Mar 22, 2016

@ajb-in

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2016

@stof re chains: I played with the existing testcase for 2.3 to see what happens with chained aliases. I ran this code:

    $container = new ContainerBuilder();

    $aDefinition = $container->register('a', '\stdClass');
    $aDefinition->setFactory(array(new Reference('b'), 'createA'));

    $bDefinition = new Definition('\stdClass');
    $bDefinition->setPublic(false);
    $container->setDefinition('b', $bDefinition);

    $container->setAlias('a_alias', 'a');
    $container->setAlias('b_alias', 'b');

    // I added these three lines:
    $container->setAlias('c_alias', 'b');
    $container->setAlias('d_alias', 'c_alias');
    $container->setAlias('e_alias', 'd_alias');

    $this->process($container);

and it 'sploded on 2.3:

There was 1 error:
1) Symfony\Component\DependencyInjection\Tests\Compiler\ReplaceAliasByActualDefinitionPassTest::testProcess
Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: Unable to replace alias "d_alias" with actual definition "c_alias".
/code/symfony/src/Symfony/Component/DependencyInjection/Compiler/ReplaceAliasByActualDefinitionPass.php:48
/code/symfony/src/Symfony/Component/DependencyInjection/Compiler/ReplaceAliasByActualDefinitionPass.php:63
/code/symfony/src/Symfony/Component/DependencyInjection/Tests/Compiler/ReplaceAliasByActualDefinitionPassTest.php:69
/code/symfony/src/Symfony/Component/DependencyInjection/Tests/Compiler/ReplaceAliasByActualDefinitionPassTest.php:40
Caused by
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "c_alias".
@ajb-in

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2016

It explodes also if I change the extra part to this:

    // I added these two lines:
    $container->setAlias('c_alias', 'b_alias');
    $container->setAlias('d_alias', 'c_alias');
@tadcka

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2016

👍

@egils

This comment has been minimized.

Copy link

commented Mar 23, 2016

Lovely! 👍

@xabbuh

This comment has been minimized.

Copy link
Member

commented Mar 25, 2016

@ajb-in Aliases that refer to other aliases should have been resolved in the ResolveReferencesToAliasesPass which is run before the ReplaceAliasByActualDefinitionPass.

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 27, 2016

As this is not a bug fix, it should be merged in master, not 2.3

@ajb-in

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2016

@fabpot When I asked in #symfony I was told that it could count as a performance bug, so that's why I went with 2.3; @javiereguiluz seemed to agree since he labeled this PR as 'bug'.
I'll rebase it onto master if needed, of course.

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 27, 2016

Sorry about the confusion. It's true that we've been more willing to merge PRs that do not strictly fix bugs in maintained branches (and I probably merged some myself), but I think this is a mistake. First, we want old branches to be as stable as possible, and merging non-bug fixes can introduce subtle regressions (happened in the past). Then, improving performance in new versions is a great incentive for people to upgrade, which is always a good idea.

/cc @symfony/deciders

@Tobion

This comment has been minimized.

Copy link
Member

commented Mar 28, 2016

I'd prefer to merge refactorings like this one in maitainance branches as it will make maintaining branches easier when they are more similar and do not diverge that much just because of refactorings.

@Tobion

This comment has been minimized.

Copy link
Member

commented Mar 28, 2016

And introducing suble behavior changes should not be an argument. It just means we do not have enough tests (or it's really an edge case that wasn't meant to be like this). E.g. the refactoring in the templating parser actually revealed that people use absolute paths that wasn't intended that way.

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 30, 2016

@ajb-in Can you rebase on current 2.3? Thanks.

@weaverryan

This comment has been minimized.

Copy link
Member

commented Mar 30, 2016

In general, I'd prefer things like this to be merged into master. If this causes a subtle behavior change that isn't caught by our tests, that should be our problem, not a problem that is discovered by users in a patch update. We need users to have very high confidence in upgrading patch versions (otherwise, they may stay on insecure versions!)

But it's not black and white - a performance issue could be a bug, so we can do anything here.

@ajb-in ajb-in force-pushed the ajb-in:2.3 branch from 09971ea to 7e9f36e Mar 31, 2016

ajb-in added a commit to ajb-in/symfony that referenced this pull request Mar 31, 2016

Optimize ReplaceAliasByActualDefinitionPass
Previous implementation passed over every alias and every definition, for every
alias (n*(n+m) runtime). New implementation passes once over all aliases and
once over all definitions (n+m).

Also removing needless "restart" logic --- it has no real effect in either case.

Includes fixes after review: symfony#18265
@ajb-in

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2016

Rebased on latest 2.3; also squashed, added a comment, and added a plug to this review on the commit message :3

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 31, 2016

@ajb-in Can you remove the reference in the commit message? We already have everything in place to automatically get that in Git notes.

@ajb-in ajb-in force-pushed the ajb-in:2.3 branch from 7e9f36e to f08036a Mar 31, 2016

@ajb-in

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2016

@fabpot Done; just wanted to give some due credit :3

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 31, 2016

Tests are broken apparently.

Optimize ReplaceAliasByActualDefinitionPass
Previous implementation passed over every alias and every definition, for every
alias (n*(n+m) runtime). New implementation passes once over all aliases and
once over all definitions (n+m).

Also removing needless "restart" logic --- it has no real effect in either case.

@ajb-in ajb-in force-pushed the ajb-in:2.3 branch from f08036a to ab8dc0c Mar 31, 2016

@ajb-in

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2016

@fabpot I forgot to rename a variable =P
Fixed it now

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 31, 2016

Thank you @ajb-in.

@fabpot fabpot merged commit ab8dc0c into symfony:2.3 Mar 31, 2016

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 31, 2016

bug #18265 Optimize ReplaceAliasByActualDefinitionPass (ajb-in)
This PR was merged into the 2.3 branch.

Discussion
----------

Optimize ReplaceAliasByActualDefinitionPass

| Q             | A
| ------------- | ---
| Branch?       | 2.3
| Bug fix?      | yes (performance)
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Previous implementation passed over every definition for every alias (n*m runtime).
New implementation passes once over all aliases and once over all definitions (n+m).
Also removing needless "restart" logic.

Commits
-------

ab8dc0c Optimize ReplaceAliasByActualDefinitionPass
@ajb-in

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2016

🍻 Thanks to everyone!

solivier pushed a commit to solivier/FOSElasticaBundle that referenced this pull request May 18, 2016

solivier
Fix tests
* Test were broken because of this PR introduced in symfony 2.3:
symfony/symfony#18265

* We were passing an object instead of a string.

@solivier solivier referenced this pull request May 18, 2016

Merged

Fix tests #1077

solivier added a commit to solivier/FOSElasticaBundle that referenced this pull request May 18, 2016

Fix tests
* Tests were broken because of this PR introduced in symfony 2.3:
symfony/symfony#18265

* We were passing an object instead of a string.

@tadcka tadcka referenced this pull request Jun 17, 2016

Closed

Twig dependency #447

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.