WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#16927 closed task (blessed) (fixed)

Don't use XHR to load portions of Dashboard that are "hot" in the cache

Reported by: markjaquith Owned by: dd32
Milestone: 3.2 Priority: normal
Severity: normal Version:
Component: Performance Keywords: has-patch commit
Focuses: Cc:

Description

The WordPress admin Dashboard screen loads several sections using XHR. If the contents are already "hot" in the cache, we should just include it in the initial load, and skip the XHR for that section.

Attachments (4)

16927.diff (8.4 KB) - added by markjaquith 4 years ago.
16927.no-js-message.patch (711 bytes) - added by ocean90 4 years ago.
16927.2.diff (2.4 KB) - added by dd32 4 years ago.
16927.delete_transient.patch (762 bytes) - added by ocean90 4 years ago.

Download all attachments as: .zip

Change History (36)

@markjaquith4 years ago

comment:1 @markjaquith4 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed
  • Owner set to markjaquith
  • Status changed from new to accepted

First attempt. Results in massively more fluid loading experience, and an overall faster load time.

Sidenote: should we have a fragment caching helper in core? I use code like this:

$fragment = new WP_Fragment_Cache( 'some_key', $ttl);
if ( !$fragment->echo() ) {
   echo "foo";
  $fragment->save();
}

The echo function echoes if the cache is hot, else it starts the ob. And save() does ob_get_flush() and a cache set. Makes it pretty simple.

Version 0, edited 4 years ago by markjaquith (next)

comment:2 follow-up: @nacin4 years ago

Hm, I didn't expect this to require a persistent cache. All of these boxes rely on either transient or feed caching. I figured we should be able to rely on that -- do you think we can leverage that so everyone can benefit?

comment:3 @nacin4 years ago

Whoa -- okay, discovered some history here.

We already have wp_dashboard_cached_rss_widget(), but it isn't used. That's really odd -- but that sure makes things easier in implementing this. Turns out it was removed in #10133, [11613]. Reading through that ticket, it seems all we patched were the symptoms. I wonder what's changed and whether we can return to the original implementation.

comment:4 follow-up: @dd324 years ago

We already have wp_dashboard_cached_rss_widget(), but it isn't used.

I seem to recall that once upon a time, these only ajax loaded if required (like this ticket is pushing for), so I wouldn't be surprised if there's functions around for it already. It might've been lost in the UI overhaul..

comment:5 in reply to: ↑ 4 @nacin4 years ago

Replying to dd32:

We already have wp_dashboard_cached_rss_widget(), but it isn't used.

I seem to recall that once upon a time, these only ajax loaded if required (like this ticket is pushing for), so I wouldn't be surprised if there's functions around for it already. It might've been lost in the UI overhaul..

As linked, it was lost in #10133. :-)

comment:6 @dd324 years ago

As linked

For some reason I think I skipped that entire line while reading your reply...

comment:7 @nacin4 years ago

For QuickPress, we should also now revert part of #10917. I don't really see a purpose, other than avoiding a few queries, for having made QuickPress be rendered via XHR. I imagine it was done for parity, but ideally we're moving in the opposite direction now.

So, the purpose of this ticket should be to: Restore behavior for feeds changed in #10133, and remove the XHR behavior introduced in #10917. The first part requires investigation as to the true errors that were reported then, while the second is pretty straightforward.

comment:8 in reply to: ↑ 2 @markjaquith4 years ago

Replying to nacin:

Hm, I didn't expect this to require a persistent cache. All of these boxes rely on either transient or feed caching. I figured we should be able to rely on that -- do you think we can leverage that so everyone can benefit?

Can use transient instead of object cache, sure. Caching the whole output will be most efficient.

comment:9 @nacin4 years ago

In principle I agree with Mark that we should cache the whole output. But after looking at it, I'm not sure.

By caching final output, we end up with potentially staggered caches. The feeds are cached for 12 hours in a transient. If we cache the output on top of that, we can't cache it for long (Mark's patch makes it under a half-hour), or we could end up with a significant overlap that causes the data to be even more out of date than 12 hours.

We need this cache to be as hot as possible for it to be a good impact on perceptive speed. The dashboard really isn't visited that often. Perhaps on login, when the cache has the least chance of being hot.

It's a trade-off: The bit of time it takes to process the data of the feed is worth it when being able to hold the feed in cache for much longer. So I'd rather return to using wp_dashboard_cached_rss_widget(). This also prevents a number of widget args from busting the cache (only number of items, really).

comment:10 @nacin4 years ago

(In [17743]) Don't load the QuickPress widget via XHR. see #16927. Reverts [16725] as it's no longer needed. Reverts part of [14815], which introduced the behavior, see #10917.

comment:11 @nacin4 years ago

I reverted [11613/trunk/wp-admin/includes/dashboard.php] and all I can say is, "whoa." Tried it on a pretty slow shared host too. Loads super fast.

I imagine some JS changed in [11613] will also need to change back, but my one-file revert works like a charm here.

comment:12 @nacin4 years ago

(In [17744]) Add line height to QuickPress media buttons. Prevents jitters as the images load. see #16927.

comment:13 @nacin4 years ago

I know the best way to avoid a repaint is to include width/height attributes, but adding a line height was much cleaner than new arguments for _media_button().

comment:14 @markjaquith4 years ago

Go ahead with a feed-cache based solution. It's likely imperceptibly slower, and will be more predictable.

comment:15 @nacin4 years ago

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

(In [17764]) Don't load dashboard widgets via XHR when the feed cache is hot. What a difference it makes, too. Reverts part of [11613], fixes #16927.

comment:16 @nacin4 years ago

Per azaozz, the JS is fine.

comment:17 follow-up: @nacin4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

The memory footprint of SimplePie is pretty ridiculous. The dashboard went from 23.6MB to 30.3MB. Going to consider attempting an output-to-transient solution.

comment:18 @ocean904 years ago

  • Keywords dev-feedback removed

16927.no-js-message.patch will re-add a message for no-js users. Maybe we can re-think #13516 here too.

comment:19 @markjaquith4 years ago

In [17782]:

Provide no-js message on the dashboard when the cache is not hot and an XHR is required. props ocean90. see #16927

comment:20 in reply to: ↑ 17 @westi4 years ago

  • Keywords close added

Replying to nacin:

The memory footprint of SimplePie is pretty ridiculous. The dashboard went from 23.6MB to 30.3MB. Going to consider attempting an output-to-transient solution.

I think this is going to have to wait for 3.3 and should go in a new ticket?

comment:21 @markjaquith4 years ago

Let's try to bump the memory limit for the Dashboard and call this done for 3.2. 48MB?

comment:22 @nacin4 years ago

I'm not comfortable loading 30.3MB -- with server configurations and a few plugins, that goes above 32MB very easily. We either need a second layer of caching to avoid loading SimplePie on the dashboard, or we need to ask for more memory on the dashboard.

I don't know which is easier. Second layer of caching might not be difficult to bake into the widget caching function, but I also know that trying to add transient caching drove both Mark and I mad initially.

Asking for more memory is easy, but that's actually uncharted territory. We currently only bump the memory limit to WP_MEMORY_LIMIT or WP_MAX_MEMORY_LIMIT -- we want something in between, which means we need to check it against both of those constants. Given the pedanticism in #14889, introducing a middle-ground, hard-coded value of, say, 48MB might mean we need to revisit our memory comparison handling.

@dd324 years ago

comment:23 @dd324 years ago

attachment 16927.2.diff added

Adds a transient around the cachable widgets, caches the output only (using an output buffer).

Memory on the cold & hot cache pages ends up the same: 26,384,776 bytes (whereas, it was ~33M before for the hot cache)

comment:24 @dd324 years ago

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

In [18228]:

Cache the Dashboard RSS Widgets HTML output to reduce the memory footprint of including SimplePie. Fixes #16927

comment:25 @ocean904 years ago

  • Keywords needs-patch added; has-patch close removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Transient should be cleared if settings are changed.

comment:26 @ocean904 years ago

  • Keywords has-patch added; needs-patch removed

$cache_key = 'dash_' . md5( $widget_id ); should be okay too, so we can delete the transient in the control function.

comment:27 @nacin4 years ago

  • Owner changed from markjaquith to dd32
  • Status changed from reopened to assigned

Assigned to dd32 for review.

comment:28 @nacin4 years ago

md5 doesn't seem to be necessary unless we're worried about length of widget IDs.

I'm happy with the patch. Good solution.

I had thought that dashboard widget configuration options (like RSS feed, titles, # items, etc.) were user-specific. Lame that they are not, but it also makes this much easier to cache.

comment:29 @nacin4 years ago

  • Keywords commit added

Marking as a commit candidate, though I haven't tested.

comment:30 @dd324 years ago

I agree, Was a bit silly of me not to key it off $widget_id

comment:31 @dd324 years ago

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

In [18264]:

Key the Dashboard widget cache off the Widget ID & clear cache upon options changing. Props ocean90. Fixes #16927

comment:32 @dd324 years ago

In [18265]:

Restore the md5 in the Dash widget cache key, prevents issues with long widget ids. Props nacin. Fixes #16927

Note: See TracTickets for help on using tickets.