WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#31789 closed enhancement (fixed)

Themes: allow a pre filter in `wp_prepare_themes_for_js()`

Reported by: mattwiebe Owned by: SergeyBiryukov
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.2
Component: Themes Keywords: has-patch commit
Focuses: performance Cc:

Description

On a site like WordPress.com which has ~450 themes, wp_prepare_themes_for_js() is very slow. Unfortunately there isn't a pre_ filter that lets us short-circuit the manual labour every time it's called. Fortunately adding one is easy!

This becomes more important in the context of theme browsing in the Customizer, which needs to call this every time a different theme is previewed there.

Attachments (4)

31789.diff (1.1 KB) - added by mattwiebe 7 years ago.
31789.1.diff (1.2 KB) - added by mattwiebe 7 years ago.
31789.2.diff (1.2 KB) - added by DrewAPicture 7 years ago.
refinements
31789.3.diff (680 bytes) - added by DrewAPicture 7 years ago.
Renamed to pre_prepare_themes_for_js

Download all attachments as: .zip

Change History (16)

@mattwiebe
7 years ago

#1 @mattwiebe
7 years ago

  • Keywords has-patch added

This ticket was mentioned in Slack in #core-customize by mattwiebe. View the logs.


7 years ago

@mattwiebe
7 years ago

#3 @helen
7 years ago

  • Focuses performance added
  • Milestone changed from Awaiting Review to 4.2

Some other things are being looked at with regards to lazy loading so not going to go for this just yet, but milestoning for 4.2 so we don't lose track.

#4 @mattwiebe
7 years ago

Some other things are being looked at with regards to lazy loading so not going to go for this just yet

Lazy loading will only help speed thing up once wp-admin/customize.php comes down the pipe. None of the approaches there are making an async call for theme data, nor are any planned (that I've seen). The data is still bootstrapped, it's just that maybe the <img> tags won't be clogging things up, initially. ( #31793 )

Without this filter, I either need to hack core on wpcom to make theme browsing in the Customizer performant, or just disable the feature entirely. 8-10 second initial load times for wp-admin/customize.php just won't cut it, especially since each time you preview a theme will reload the Customizer and have a really slow load, again.

#5 @mattwiebe
7 years ago

Just to provide a bit more data, here's a set of ab results, just for loading wp-admin/customize.php

v 4.1.1
              min  mean[+/-sd] median   max
Connect:      284  299  16.2    293     347
Processing:  2876 3259 532.5   3093    5031
Waiting:     1733 2066 530.1   1888    3897
Total:       3170 3557 544.9   3392    5378

v 4.2-beta3-31930
              min  mean[+/-sd] median   max
Connect:      281   304   22.9   302    375
Processing:  9381 10776 1617.9 10423  16147
Waiting:     7477  8755 1638.2  8366  14265
Total:       9663 11080 1620.2 10733  16441

My wp.com sandbox isn't crazy powerful, but that's still really rough.

@DrewAPicture
7 years ago

refinements

#6 @DrewAPicture
7 years ago

31789.2.diff refines the approach a little bit and adjusts the filter docs for core style:

  • Docs: summary updated: what is filterable, not why
  • Docs: removes the justification, e.g. "allows you to do X because of X", as every use case is different
  • Docs: Remove the @return (unused in hook docs)
  • Simplifies the code by just passing an empty array to the filter
  • Simplify setting the current theme as the first key (since we know it's an array).

#7 @johnbillion
7 years ago

  • Keywords commit added

#8 @SergeyBiryukov
7 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 31943:

Themes: Add a filter to short-circuit wp_prepare_themes_for_js().

props mattwiebe, DrewAPicture.
fixes #31789.

#9 follow-up: @nacin
7 years ago

This is one of only two filters that are named pre_wp_*. (The other is pre_wp_nav_menu.) Of the 140 or so filters that start with wp_ do not have corresponding pre_ (no pre_wp_) filters. So while the only precedent is pre_wp_nav_menu, it feels *really* weird. Anyone against pre_prepare_themes_for_js? @sergeybiryukov @drewapicture @mattwiebe

#10 in reply to: ↑ 9 @SergeyBiryukov
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to nacin:

Anyone against pre_prepare_themes_for_js?

No objections from me.

@DrewAPicture
7 years ago

Renamed to pre_prepare_themes_for_js

#11 @DrewAPicture
7 years ago

+1 for pre_prepare_themes_for_js instead.

Done in 31789.3.diff.

#12 @helen
7 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 32246:

Rename the pre_wp_prepare_themes_for_js filter to pre_prepare_themes_for_js.

props DrewAPicture.
fixes #31789.

Note: See TracTickets for help on using tickets.