WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 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 5 years ago.
16927.no-js-message.patch (711 bytes) - added by ocean90 5 years ago.
16927.2.diff (2.4 KB) - added by dd32 5 years ago.
16927.delete_transient.patch (762 bytes) - added by ocean90 5 years ago.

Download all attachments as: .zip

Change History (36)

@markjaquith
5 years ago

#1 @markjaquith
5 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 5 years ago by markjaquith (next)

#2 follow-up: @nacin
5 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?

#3 @nacin
5 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.

#4 follow-up: @dd32
5 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..

#5 in reply to: ↑ 4 @nacin
5 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. :-)

#6 @dd32
5 years ago

As linked

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

#7 @nacin
5 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.

#8 in reply to: ↑ 2 @markjaquith
5 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.

#9 @nacin
5 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).

#10 @nacin
5 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.

#11 @nacin
5 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.

#12 @nacin
5 years ago

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

#13 @nacin
5 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().

#14 @markjaquith
5 years ago

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

#15 @nacin
5 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.

#16 @nacin
5 years ago

Per azaozz, the JS is fine.

#17 follow-up: @nacin
5 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.

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

#19 @markjaquith
5 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

#20 in reply to: ↑ 17 @westi
5 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?

#21 @markjaquith
5 years ago

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

#22 @nacin
5 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.

@dd32
5 years ago

#23 @dd32
5 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)

#24 @dd32
5 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

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

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

#27 @nacin
5 years ago

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

Assigned to dd32 for review.

#28 @nacin
5 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.

#29 @nacin
5 years ago

  • Keywords commit added

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

#30 @dd32
5 years ago

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

#31 @dd32
5 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

#32 @dd32
5 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.