WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 29 hours ago

#34676 new enhancement

Optimize bulk plugin updates

Reported by: jipmoors Owned by:
Milestone: 5.3 Priority: normal
Severity: normal Version: 4.4
Component: Upgrade/Install Keywords: has-patch needs-unit-tests shiny-updates
Focuses: performance Cc:

Description

When selecting more then one plugins to update the following things are done:

  • Maintenance mode on (if -a- plugin is active)
  • Per plugin:
  1. Get plugin info
  2. Download package
  3. Unpack package
  4. Install package
  • Maintenance mode off

Scenario:
10 plugins require updates.
Only the last one is active = requires maintenance mode to be enabled.
Server might not have the best connection to WordPress.org or other plugin resource.

This means that the site will not be available during:

  • Downloading of all plugins
  • Unpacking of all plugins
  • Installing of all plugins

This means at least (10*download request) + (10*unpacking) + (10*installing) = 30 actions.

Not including optional plugin info request * 10.
Also not including plugins that need to do delta's on tables or other upgrading scripts.

Though only one plugin actually required the site to not be available, which would be (1*installing) = 1 action.

Ideal flow:

  • Download all packages
  • Unpack downloaded packages
  • Determine per plugin if maintenance is required
  • Install plugin
  • Disable maintenance if next plugin is not active
  • Finally: disable maintenance if enabled
  • Remove all unpacked packages and downloads

I can think of a performance argument to include the unpacking of packages in the maintenance mode because it might be a strain on the server, but functionally I don't think it should be.

As far as I can see the only file that this is effectively handled in is class-wp-upgrader.php.

Attachments (6)

34676.1.patch (38.6 KB) - added by jipmoors 4 years ago.
Separated Bulk from Upgrader
34676.2.class-seperation-and-docs.diff (136.2 KB) - added by jipmoors 4 years ago.
One class per file, docs improved
34676.2.patch (49.1 KB) - added by jipmoors 4 months ago.
Refreshed patch
34676.3.patch (49.8 KB) - added by jipmoors 4 months ago.
Extended patch 2 with correct plugin counter (1/5...5/5)
34676.4.patch (53.6 KB) - added by jipmoors 4 months ago.
Extended patch 3: unpack (1/x)
34676.5.patch (54.9 KB) - added by jipmoors 4 months ago.
Extended patch 4: Added translator comments

Download all attachments as: .zip

Change History (43)

@jipmoors
4 years ago

Separated Bulk from Upgrader

#1 @jipmoors
4 years ago

  • Keywords has-patch ux-feedback needs-unit-tests added

#2 @jipmoors
4 years ago

The added patch separates the functionality of the Bulk upgrades from the Upgrader.
Because of how the skin is build up, the feedback has been modified a bit.

All the plugins that are upgraded are shown, after that you wait until all actions have been completed.
It would be preferable that these steps have their own output: downloading packages, unpacking downloads, installing plugins. I don't have any experience on dealing with these language/translation additions so guidance would be welcome.

All functionality has been maintained, only distributed differently.

Tested:

  • 1 plugin updated (update-core.php)
  • 3 plugins updated (update-core.php)
  • 1 active plugin, 2 inactive plugins updated (update-core.php)
  • 3 active plugins updated (update-core.php)
  • 1 plugin updated (plugins.php)
  • 1 theme updated
  • 3 themes updated
  • language updates

#3 @DrewAPicture
4 years ago

Looking at 34676.1.patch, the initial things in terms of documentation that jump out at me are:

  • The version should 4.5.0 (we aren’t going to introduce something like this in a point release)
  • All of the property DocBlocks should have descriptions
  • The phpDoc tag description don’t need to be aligned
  • Several parameters don’t have descriptions at all
  • There should only be one class per file

@jipmoors
4 years ago

One class per file, docs improved

#4 @dd32
4 years ago

  • Component changed from Plugins to Upgrade/Install

#5 @obenland
3 years ago

  • Keywords shiny-updates added

#6 @jrf
3 years ago

Related: #36618

Last edited 3 years ago by ocean90 (previous) (diff)

#7 @ocean90
3 years ago

@jipmoors Looking at 34676.2.class-seperation-and-docs.diff: In src/wp-admin/update-core.php you changed include_once( ABSPATH . 'wp-admin/includes/class-wp-upgrader.php' ); to require_once( ABSPATH . 'wp-admin/includes/class-language-pack-upgrader.php' ); but Language_Pack_Upgrader extends WP_Upgrader. Was this a mistake or how did you solve this?

#8 follow-up: @jipmoors
3 years ago

@ocean90 it seems I totally screwed that patch up. I must say I'm not so fluent at SVN at this point ;)
There are many files missing and that logic is broken as you pointed out.

Base functionality is all in the first patch, the second one is just separating files.
I don't mind re-doing the patch if it has any added value.
Otherwise happy to refresh/improve the first patch and document as required.

#9 @jipmoors
3 years ago

Revisiting this ticket again I think I also would like to rethink the architectural choice made and probably chance the approach a bit.

#10 in reply to: ↑ 8 @ocean90
3 years ago

Replying to jipmoors:

@ocean90 it seems I totally screwed that patch up. I must say I'm not so fluent at SVN at this point ;)

No worries. :) The split is now handled in #36618.

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


2 years ago

#12 @karmatosed
2 years ago

Noting this needs a patch refresh, once we have the patch then it would be good to see what is being suggested. Showing some screenshots would also be really good, to help people see.

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


15 months ago

#14 @karmatosed
15 months ago

The design team would still love to give feedback on this. Could we have screenshots of the refreshed patch please?

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


4 months ago

#16 @karmatosed
4 months ago

  • Keywords ux-feedback removed

#17 @airathalitov
4 months ago

This is how I see it:

Logic for updating one plugin:

  • Download plugin
  • Unpack plugin
  • If plugin is active {
  • Turn on maintenance mode
  • Install plugin
  • Disable maintenance mode }
  • Else if plugin is not active {
  • Install plugin (without maintenance mode) }
  • Delete old files

Logic for updating multiple plugins:

  • Download all plugins
  • Unpack all plugins
  • Check status of each plugin (active|inactive)
  • Group plugins into 2 groups by status (active|inactive)
  • Install all inactive plugins (without enabling maintenance mode)
  • Enable maintenance mode
  • Install all active plugins
  • Disable maintenance mode
  • Delete old files

This algorithm should work faster. And you do not need to constantly switch the maintenance mode.

#18 @SergeyBiryukov
4 months ago

  • Keywords needs-refresh added

#19 @SergeyBiryukov
4 months ago

  • Milestone changed from Awaiting Review to Future Release

#20 @jipmoors
4 months ago

Before - 1 update (active)

https://i.imgur.com/NxdvYwh.png
https://i.imgur.com/RRvvCA7.png

After - 1 update (active)

https://i.imgur.com/uFgMvEt.png
https://i.imgur.com/nihCbbZ.png

Before - 2 updates (1 active, 1 inactive)

This will activate maintenance mode surrounding the entire process.
https://i.imgur.com/PeTRT0i.png
https://i.imgur.com/FQOtBZc.png

After - 2 updates (1 active, 1 inactive)

This will only activate maintenance mode for the active plugin.

https://i.imgur.com/fh5jyXX.png
https://i.imgur.com/UK6o7Z6.png

@jipmoors
4 months ago

Refreshed patch

#21 @jipmoors
4 months ago

  • Keywords needs-refresh removed

The refreshed patch improved the visual representation of the bulk updates.

It does not implement any changes for languages (which the first patch did).
So it just does it for plugins and themes.

#22 @airathalitov
4 months ago

@jipmoors
Could you test your patch on this example sequence and show a screenshot:

  • active plugin
  • disabled plugin
  • active plugin
  • disabled plugin
  • active plugin

#23 @SergeyBiryukov
4 months ago

  • Milestone changed from Future Release to 5.3

#24 @jipmoors
4 months ago

Running the suggested situation, I noticed that the identifier for which plugin is being installed is based on the list of plugins as they appear on the plugin page (and selector on the upgrade page).

I think it would be clearer to make it sequential.

See the screenshots:
https://i.imgur.com/AHz55EK.png
https://i.imgur.com/Rfnxk96.png
https://i.imgur.com/6H19tpC.png

#25 @jipmoors
4 months ago

Also note that for an upgrade of plugins/themes on a multisite network-admin, maintenance mode will be enabled around all updates - as it is implemented in the current situation.

#26 @jipmoors
4 months ago

@airathalitov I have not separated the deletion of old files from the flow, as it would require separating more logic from bulk and non-bulk installs. This is something I wanted to reduce for this first improvement.

Disk usage

Though this point has me realizing that this new method of walking through the flow has a potential disk-space issue, which was not as prominent before.

As all downloads and unzips are taking place beforehand, the space requires has increased to the total amount of all plugins being upgraded. Before this was only the size of the biggest plugin.

As a site running at near full disk is at risk from being unavailable for many other reasons, I don't think it should be an instant blocker. This concern should be taken into account in determining if it's acceptable for merge.

[ Linking this comment for consult from the hosting-providers slack channel ]

#27 @jipmoors
4 months ago

As this improvement has not been raised as an actual problem, I want to stress that not having this improvement would be a perfectly acceptable outcome for me as well.

I personally think the user experience will benefit and having less maintenance mode seconds is an improvement. I also want to have a full consideration, especially on the implications I am unaware of or you have better understand off. So I'm still very much behind this improvement :)

This ticket was mentioned in Slack in #hosting-community by jip. View the logs.


4 months ago

#29 @airathalitov
4 months ago

The patch is working great. Thanks!

I thought about disk usage before.

Did you delete zip archives of plugins after unpacking each of them?

For example If we have 10 plugins to update It would take not bigger than 100MB. I think it's not so big and everything should be good.

Last edited 4 months ago by airathalitov (previous) (diff)

#30 @jipmoors
4 months ago

Deleting the zip is also working the same as before, when it's a local file it will not be removed - otherwise the unpacking mechanism removes the file when it's done.

@jipmoors
4 months ago

Extended patch 2 with correct plugin counter (1/5...5/5)

#31 @jipmoors
4 months ago

The one thing I have not been able to fix is that the unpacking feedback shows this:

Unpacking the update…
Unpacking the update…
Unpacking the update…
Unpacking the update…
Unpacking the update…

Which is just one line per plugin being unpacked.
As this feedback is given at a place where the context is missing totally, I don't see an easy way of improving on this.

#32 @airathalitov
4 months ago

Maybe should it contain package name?

Unpacking plugin-name.x.x.x.zip…

instead of

Unpacking the update…

Example:

Unpacking gutenberg.5.7.0.zip…
Unpacking hello-dolly.1.6.zip…
Unpacking wordpress-seo.11.2.1.zip…
...

It looks more informative.

#33 @jipmoors
4 months ago

Currently only the tmp file is available, which is not matching the zip or plugin name. So I decided not to add that to it.

#34 @airathalitov
4 months ago

Or another variant

Unpacking the update… (1/5)
Unpacking the update… (2/5)
Unpacking the update… (3/5)
Unpacking the update… (4/5)
Unpacking the update… (5/5)

And you don't need to change the translation strings.

Last edited 4 months ago by airathalitov (previous) (diff)

@jipmoors
4 months ago

Extended patch 3: unpack (1/x)

#35 @jipmoors
4 months ago

Was able to implement the Unpacking ... (1/5) suggestion, had to add a translation string which holds the counting as this could be placed differently for other languages.

Just realized I forgot translator comments, will add those.

@jipmoors
4 months ago

Extended patch 4: Added translator comments

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


8 days ago

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


29 hours ago

Note: See TracTickets for help on using tickets.