Problem/Motivation

Follow-up to #3343827: Update FixtureManipulator to work with InstalledPackagesList, real composer show command where we change our manipulator to use real composer commands

Right now if you do this. 

    (new ActiveFixtureManipulator())
      ->addPackage([
        'name' => "drupal/dependency",
        'version' => '9.8.0',
        'type' => 'drupal-library',
      ])
      ->addPackage([
        'name' => "drupal/semver_test",
        'version' => '8.1.0',
        'type' => 'drupal-module',
      ])
      ->commitChanges();

It will result in 2 calls to `composer require` when it could be done in 1

Proposed resolution

Change Fixture Manipulator to only make as many Composer as needed.

We should be able to group calls for `require` and `remove` commands. We would have to also group by dev and non-dev packages.

Issue fork automatic_updates-3345222

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Title: Optimize Composer call in Fixture Manipulator » Optimize Composer calls in Fixture Manipulator
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Optimize Composer calls in Fixture Manipulator » [PP-1] Optimize Composer calls in Fixture Manipulator
Status: Active » Postponed

#3343827: Update FixtureManipulator to work with InstalledPackagesList, real composer show command needs to land before this can be actionable.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-1] Optimize Composer calls in Fixture Manipulator » Optimize Composer calls in Fixture Manipulator
Priority: Normal » Major
Status: Postponed » Active
Issue tags: +sprint

#3343827: Update FixtureManipulator to work with InstalledPackagesList, real composer show command landed! And since this could help address #3338789-28: Random failure: "PHP temp directory (/tmp) does not exist or is not writable to Composer.", pulling into the sprint…

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Optimize Composer calls in Fixture Manipulator » Optimize Composer calls in FixtureManipulator
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#3338789: Random failure: "PHP temp directory (/tmp) does not exist or is not writable to Composer."
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Performance
omkar.podey’s picture

omkar.podey commented
Assigned: Unassigned » omkar.podey

omkar.podey opened merge request !803

omkar.podey opened merge request !805

omkar.podey’s picture

omkar.podey commented

Unfortunately, the test timings are not consistent and it seems that grouping require calls don't help much.

omkar.podey’s picture

omkar.podey commented

The conclusion.

  1. So, as a last resort i tried to handle all addPackage() calls as group calls but since the test times are pretty much similar to the 3.0.x baseline it seems to make little to no difference to the performance so complicating it doesn't make sense.
       unit test - same
   MR vs 3.0.x
   kernel 4.23 vs 4.21
   build 8.14 vs 8.11
   functional 2.42 vs 2.43
  2. But i also think that we could simplify addPackage() with same structure that i used to make group calls possible, Thoughts?
  3. Also, maybe i missed something crucial with the optimisation?
omkar.podey’s picture

omkar.podey commented
Assigned: omkar.podey » Unassigned
Status: Active » Needs review
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Priority: Major » Minor
Status: Needs review » Needs work
Issue tags: -core-post-mvp, -sprint, -Performance +core-post-mvp Performance

@omkar.podey thanks for working on this. Yes, I don't think with the minimal performance this something we should probably spend anymore time on.

I think we can leave it open from now but set the priority to minor removing the sprint. If someone really wants to tackle this they could try to figure how to make it better they are willing to try.

but I think to if we were to commit a fix for this it could not make the class that much harder to understand. So I am doubtful that can be done.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Issue tags: -core-post-mvp Performance +core-post-mvp, +Performance
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Huh, it feels like comments of mine have been deleted from this issue? 😳

I'd swear we've had a conversation both via audio and here with at least @tedbow about closing this instead of leaving it open like #14 says since if it's not worth doing now, it never will be.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » tedbow

Also, the April 6 comment at #14 was marked as "new" for me here, which seems impossible? 🤪

Either way, assigning to @tedbow for hopefully marking this Closed (works as designed) as I suggested in #16.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs work » Closed (works as designed)

@Wim Leers I think we might have chatted in slack