Make WordPress Core

Opened 8 years ago

Last modified 22 months ago

#34676 assigned enhancement

Optimize bulk plugin updates

Reported by: jipmoors's profile jipmoors Owned by: francina's profile francina
Milestone: Future Release Priority: normal
Severity: normal Version: 4.4
Component: Upgrade/Install Keywords: has-patch needs-unit-tests shiny-updates early needs-refresh
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 (9)

34676.1.patch (38.6 KB) - added by jipmoors 8 years ago.
Separated Bulk from Upgrader
34676.2.class-seperation-and-docs.diff (136.2 KB) - added by jipmoors 8 years ago.
One class per file, docs improved
34676.2.patch (49.1 KB) - added by jipmoors 5 years ago.
Refreshed patch
34676.3.patch (49.8 KB) - added by jipmoors 5 years ago.
Extended patch 2 with correct plugin counter (1/5...5/5)
34676.4.patch (53.6 KB) - added by jipmoors 5 years ago.
Extended patch 3: unpack (1/x)
34676.5.patch (54.9 KB) - added by jipmoors 5 years ago.
Extended patch 4: Added translator comments
34676.5.diff (69.6 KB) - added by bookdude13 4 years ago.
Refreshed patch 5
34676.6.patch (69.6 KB) - added by bookdude13 4 years ago.
Numbering wrong before, corrected
update_some_active_links_broken.PNG (43.1 KB) - added by bookdude13 4 years ago.
Update page, some active plugins and some not.

Download all attachments as: .zip

Change History (55)

@jipmoors
8 years ago

Separated Bulk from Upgrader

#1 @jipmoors
8 years ago

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

#2 @jipmoors
8 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
8 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
8 years ago

One class per file, docs improved

#4 @dd32
8 years ago

  • Component changed from Plugins to Upgrade/Install

#5 @obenland
8 years ago

  • Keywords shiny-updates added

#6 @jrf
8 years ago

Related: #36618

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

#7 @ocean90
8 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
8 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
8 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
8 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.


7 years ago

#12 @karmatosed
7 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.


6 years ago

#14 @karmatosed
6 years 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.


5 years ago

#16 @karmatosed
5 years ago

  • Keywords ux-feedback removed

#17 @airathalitov
5 years 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
5 years ago

  • Keywords needs-refresh added

#19 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to Future Release

#20 @jipmoors
5 years 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
5 years ago

Refreshed patch

#21 @jipmoors
5 years 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
5 years 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
5 years ago

  • Milestone changed from Future Release to 5.3

#24 @jipmoors
5 years 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
5 years 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
5 years 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
5 years 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.


5 years ago

#29 @airathalitov
5 years 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 5 years ago by airathalitov (previous) (diff)

#30 @jipmoors
5 years 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
5 years ago

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

#31 @jipmoors
5 years 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
5 years 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
5 years 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
5 years 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 5 years ago by airathalitov (previous) (diff)

@jipmoors
5 years ago

Extended patch 3: unpack (1/x)

#35 @jipmoors
5 years 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
5 years ago

Extended patch 4: Added translator comments

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


5 years ago

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


5 years ago

#38 @desrosj
4 years ago

  • Keywords early needs-refresh added
  • Milestone changed from 5.3 to 5.4

Looks like the latest patch needs a refresh and this still needs unit tests.

With 5.3 Beta 1 in 3 days, I am going to punt this to 5.4. This should probably land early in the cycle to allow for some soak time and iteration.

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


4 years ago

#40 @davidbaumwald
4 years ago

  • Milestone changed from 5.4 to Future Release

This ticket was discussed during the #core bug scrub today. Given this still needs a refresh and the early tag, this is being moved to Future Release. If any committer feels this can be moved back up or can own it during a specific cycle, feel free to update the milestone.

@bookdude13
4 years ago

Refreshed patch 5

@bookdude13
4 years ago

Numbering wrong before, corrected

@bookdude13
4 years ago

Update page, some active plugins and some not.

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


3 years ago

This ticket was mentioned in Slack in #core-auto-updates by hellofromtonya. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-auto-updates by audrasjb. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-auto-updates by francina. View the logs.


3 years ago

#45 @francina
3 years ago

  • Owner set to francina
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core-auto-updates by sergey. View the logs.


22 months ago

Note: See TracTickets for help on using tickets.