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

Solver function call num optimizations #3481

Merged
merged 8 commits into from Dec 1, 2014

Conversation

Projects
None yet
3 participants
@naderman
Copy link
Member

naderman commented Dec 1, 2014

Packagist's composer update is roughly 14 million function calls

  • Literals on rule are now public: Reduction of ~500k function calls
  • id on BasePackage is now public: Reduction of ~1.3m function calls
  • ruleById on RuleSet is now public: Reduction of ~500k function calls
  • Rules no longer depend on the Pool (tiny memory improvement)
Literals on rule are now public
This causes a reduction of ~500k function calls for packagist composer
update (~14 million total).
@@ -42,6 +41,8 @@ class Rule
protected $ruleHash;
public $literals;

This comment has been minimized.

@stof

stof Dec 1, 2014

Contributor

public properties should come before protected ones

This comment has been minimized.

@stof

stof Dec 1, 2014

Contributor

and you should document why it is public (performance optimization) and that it should be treated as being read-only

This comment has been minimized.

@naderman

naderman Dec 1, 2014

Member

Indeed to both :-)

@stof

This comment has been minimized.

Copy link
Contributor

stof commented Dec 1, 2014

According to blackfire.io, the most expensive calls when doing a dry-run update are RuleSet::ruleById(). Would it make sense to make RuleSet::$ruleById public as well as a performance optimization ?

Make project id public
Reduction of rougly 1.3 million function calls on packagist update
@naderman

This comment has been minimized.

Copy link
Member

naderman commented Dec 1, 2014

Yes already have that commit locally, just doing some more benchmarks. getId() is actually rather expensive also on the BasePackage.

naderman added some commits Dec 1, 2014

Make ruleById lookup table in rule set public
Saves about 500k function calls on a packagist update
@naderman

This comment has been minimized.

Copy link
Member

naderman commented Dec 1, 2014

So currently we create conflict rules for each set of versions of the same package, I wonder if there is some other way to make that more implicit to reduce memory consumption.

*
* Each element is a package id either positive for installation or
* negative meaning removal.
*

This comment has been minimized.

@stof

stof Dec 1, 2014

Contributor

you should add a message saying it should be treated as read-only. For instance, Doctrine makes it proeminent in the phpdoc: https://github.com/doctrine/doctrine2/blob/30bf192cf4a7e0289ab113f44e80b16e6e02e456/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php#L208

This comment has been minimized.

@naderman
@@ -160,11 +169,6 @@ public function isEnabled()
return !$this->disabled;
}
public function getLiterals()

This comment has been minimized.

@stof

stof Dec 1, 2014

Contributor

This should maybe be kept (at least temporarily) to avoid BC breaks if a plugin rely on the method

This comment has been minimized.

@naderman

naderman Dec 1, 2014

Member

Good point.

/**
* Lookup table for rule id to rule object
*
* @var array

This comment has been minimized.

@stof

stof Dec 1, 2014

Contributor

Rule[] would be better for IDE support

@stof

This comment has been minimized.

Copy link
Contributor

stof commented Dec 1, 2014

So currently we create conflict rules for each set of versions of the same package, I wonder if there is some other way to make that more implicit to reduce memory consumption.

it may be done separately though (it will also be easier to review).

But I agree that it seems like a good place to look for improvements.

@@ -76,11 +82,6 @@ public function count()
return $this->nextRuleId;
}
public function ruleById($id)

This comment has been minimized.

@stof

stof Dec 1, 2014

Contributor

should be kept for BC too

@naderman

This comment has been minimized.

Copy link
Member

naderman commented Dec 1, 2014

Indeed, I'll probably leave at this for now on this PR, and open a new one for further changes.

@naderman naderman changed the title [WIP] Solver optimizations Solver optimizations Dec 1, 2014

@stof

This comment has been minimized.

Copy link
Contributor

stof commented Dec 1, 2014

this looks good to me.

Btw, you can probably cancel a bunch of Travis builds from the queue to keep only the latest one to get feedback faster about the final state of the PR.

@naderman naderman changed the title Solver optimizations Solver function call num optimizations Dec 1, 2014

@naderman

This comment has been minimized.

Copy link
Member

naderman commented Dec 1, 2014

@stof indeed, just did, only most recent ones running now

Seldaek added a commit that referenced this pull request Dec 1, 2014

@Seldaek Seldaek merged commit f291bf6 into composer:master Dec 1, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment