WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 5 months ago

Last modified 5 months ago

#25073 closed enhancement (fixed)

Heartbeat performance and API improvements

Reported by: evansolomon Owned by: azaozz
Milestone: 3.8 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

There are a few separate changes here:

  • Make some easy performance improvements
  • Add a new API method
  • Cleanup some style

Performance
Three changes here. Linked JSPerf results are from Chrome Canary (v 31).

  1. Cache $(document). It gets used a bunch of times. 15x faster
  2. Get rid of a few unnecessary closures in callbacks. Stuff like asyncMethod( function() { callback() } ) is simplified to asyncMethod( callback ). 4x faster
  3. Use Date.now() instead of (new Date).getTime() and remove the unnecessary time() function (its optional argument was never used). 2x faster

API
I made connect() a method on the Heartbeat instance. I've wanted to trigger a connect event a few times without waiting for the next scheduled tick to fire, which I think is a reasonable use case. Your mileage may vary.

Style

  • Explicitly use window when referencing globals (ajaxurl, pagenow, wp).
  • Remove a stray leading space.
  • Remove an unreachable break.

Attachments (10)

25073.diff (6.1 KB) - added by evansolomon 8 months ago.
25073.2.patch (4.8 KB) - added by azaozz 7 months ago.
25073.3.patch (23.0 KB) - added by azaozz 7 months ago.
25073-4.patch (27.7 KB) - added by azaozz 6 months ago.
25073-5.patch (28.7 KB) - added by azaozz 6 months ago.
25073-6.patch (28.3 KB) - added by azaozz 5 months ago.
25073-7.patch (1.9 KB) - added by azaozz 5 months ago.
25073-cache-document.patch (460 bytes) - added by TobiasBg 5 months ago.
Replace another $(document)
25073-8.patch (3.7 KB) - added by azaozz 5 months ago.
25073-9.patch (3.0 KB) - added by azaozz 5 months ago.

Download all attachments as: .zip

Change History (50)

evansolomon8 months ago

comment:1 follow-up: johnbillion8 months ago

Nice work Evan.

Is it not safest to keep breaks in switch statements even if they're not reachable? It aids clarity, and if the code is altered down the line it may introduce bugs due to unintentional fallthrough.

comment:2 in reply to: ↑ 1 evansolomon8 months ago

Replying to johnbillion:

Is it not safest to keep breaks in switch statements even if they're not reachable? It aids clarity, and if the code is altered down the line it may introduce bugs due to unintentional fallthrough.

I could understand that argument and wouldn't object if someone wanted to merge this patch with that change. Personally, I would always choose to remove code that can never run, but I can see how this is a potentially special case.

comment:3 azaozz8 months ago

  • Cc carldanley added

comment:4 follow-up: carldanley8 months ago

For reference, check this file out: http://core.trac.wordpress.org/attachment/ticket/23216/23216-heartbeat-cleanup.5.diff. Will comment more on your patch in a little bit, busy atm; just didn't want to forget.

comment:5 carldanley8 months ago

Would be cool to see unit tests for this once the patch goes through.

comment:6 azaozz8 months ago

...keep breaks in switch statements even if they're not reachable? It aids clarity...

Think so too, we do that in PHP in many places.

Some notes on 25073.diff:

  • Date.now() doesn't work in IE < 9.
  • Perhaps we can add public method to trigger a connection immediately/out of order but keep wp.heartbeat.connect() private. One of the main points of heartbeat is not to overwhelm the server with too many connections. That's why it has a queue, to aggregate the data before sending it. On the other hand it's tempting to use it as a "pure" transport, i.e. when the user clicks something, the result of the click is sent to the server immediately. However in many cases that "click" doesn't have to be send immediately, for example we can use heartbeat to save postbox positions and open/close state, number of columns on the Dashboard, etc. a bit later.
  • There is a (big) structural/cleanup patch by @carldanley and some discussion on #23216. Several of the enhancements there are the same as in 25073.diff.

comment:7 kadamwhite8 months ago

  • Cc kadamwhite@… added

comment:8 azaozz8 months ago

  • Cc kadamwhite@… removed

Other TO-DOs for heartbeat:

  1. Ensure a "minimum connection rate" or completely remove the "skipping". This will depend on feedback from the hosting companies. Some alternatives:
    • Remove the "skipping" if there is no increased resource usage in 3.6 or if it's insignificant. As users spend most time on the Edit Post screen and heartbeat doesn't skip there (because of the post lock checks), any negative impact to shared hosting accounts should be measurable.
    • If the impact is more significant, only skip every other "beat" or ensure that heartbeat connects at least once every 45 (or 60) sec. when the user is active/window has focus.
  2. Consider completely stopping heartbeat if the user is inactive for 15 (or 10) min. This will effectively release post locks. If the user becomes active later, heartbeat runs straight away so any notifications (lock taken over, login expired, etc.) will be shown almost immediately.
  3. Increase the "blurred" interval to 150 sec. so when a user has several tabs/windows open, heartbeat doesn't connect excessively. This will require changing the post locks expiration from 2 to 3 min.

comment:9 azaozz8 months ago

  • Cc kadamwhite@… added

comment:10 in reply to: ↑ 4 evansolomon8 months ago

Replying to carldanley:

For reference, check this file out: http://core.trac.wordpress.org/attachment/ticket/23216/23216-heartbeat-cleanup.5.diff. Will comment more on your patch in a little bit, busy atm; just didn't want to forget.

http://s.evansolomon.me/image/1E261s1H0W2K/content

azaozz7 months ago

comment:12 azaozz7 months ago

In 25073.2.patch:

  • Fix resetting the next connection time when changing the interval.
  • Prevent opening two concurrent connections.
  • Introduce connectNow() that connects immediately. If a connection is currently in progress, it schedules the next connection immediately after.

azaozz7 months ago

comment:13 azaozz7 months ago

In 25073.3.patch:

  • Merge the improvements from @carldanley and @evansolomon.
  • Add connectNow() that resets the internal timing and connects immediately.
  • Set the "blurred" interval back to 120 sec. and set the post lock expiration time to 150 sec.
  • Stop heartbeat after the user has been inactive for 15 min. (this will also release post locks). Restart when the user becomes active again.

Still to consider: remove the "skipping" and set the default interval to 60 sec. Will simplify the usage, there will be no need to set timers to send something.

comment:14 nacin7 months ago

As I suggested last week in IRC (I think), I definitely think this should wait for early 3.8 due to the number of changes and the two weeks we have to kick 3.7 out the door. The autosave improvements suggested in #25272 would then come at the same time. It's just too much churn and too little time to test this.

azaozz6 months ago

comment:15 azaozz6 months ago

25073-4.patch contains all from 25073.3.patch plus:

  • Remove the "skipping" when no data to send.
  • Change the default interval to 60 sec.
  • Add the changes to setErrorState to detect HTTP 503 errors.
  • Clean up adding the action for _admin_notice_post_locked.

azaozz6 months ago

comment:16 azaozz6 months ago

In 25073-5.patch: some cleanup and more inline comments.

comment:17 DJPaul6 months ago

  • Cc djpaul@… added

comment:18 adamsilverstein5 months ago

  • Cc adamsilverstein@… added

azaozz5 months ago

comment:19 azaozz5 months ago

In 25073-6.patch:

  • Some cleanup of the previous few patches.
  • Removed stopping the "beat" after 15 min.

The main behavior change in heartbeat is removing the skipping and instead making the default interval 60 sec. Anything that uses heartbeat can still set the interval to 5, 15, 30 or 60 sec. either from JS or PHP.

The other proposed change is to completely stop the "beat" after a user has been inactive for 15|20|30 min. This would work similarly to a screensaver or a laptop going to "sleep". However this would change post locks behavior: heartbeat would stop effectively releasing the post lock. When the user becomes active again by either focusing the browser tab/window, typing, or moving the mouse, heartbeat will fire immediately.

comment:20 azaozz5 months ago

In 26169:

Heartbeat: clean up code style, better naming, better code structure. Props carldanley, props evansolomon.
Changes:

  • Add connectNow() public method to trigger a connection immediately.
  • Remove the "skipping" when no data to send.
  • Change the default interval to 60 sec.
  • Fix resetting the next connection time when changing the interval.

See #25073.

comment:21 azaozz5 months ago

Related: #26050

azaozz5 months ago

comment:22 azaozz5 months ago

In 25073-7.patch:

  • Add "sleep" functionality triggered after 30 min. of inactivity. This will stop heartbeat until there is user activity (window.focus, mouse movement or typing).
  • If the last XHR is in progress, abort it on unload to avoid any possible collisions.

TobiasBg5 months ago

Replace another $(document)

comment:23 TobiasBg5 months ago

One $(document) was missed in [26169], see 25073-cache-document.patch.

comment:24 azaozz5 months ago

In 26276:

Heartbeat: use cached $document instead of $(document), props TobiasBg, see #25073

azaozz5 months ago

comment:25 azaozz5 months ago

25073-8.patch extends the functionality from 25073-7.patch making it possible to disable suspending from both PHP and JS.

The idea is to use "suspend" when more than one page from the same site is loaded. Then Heartbeat can be suspended immediately in all browser tabs/windows what don't have focus, leaving it running only in one tab. This will reduce the server load significantly when the user has many tabs/windows open.

comment:26 azaozz5 months ago

In 26428:

Heartbeat: introduce "suspend" functionality and enable it after 20 min. of inactivity, see #25073.

comment:27 nacin5 months ago

  • Component changed from General to Administration
  • Milestone changed from Awaiting Review to 3.8
  • Priority changed from normal to highest omg bbq
  • Severity changed from normal to blocker

Uncaught TypeError: Object #<Object> has no method 'interval' wp-auth-check.js?ver=3.8-beta-1-src:74

This prevents the auth check window from closing after a log in.

I get that we wanted to rename methods here, and possibly break compatibility, but we need to be a bit more deliberate/careful.

comment:28 azaozz5 months ago

In 26530:

Heartbeat: update wp-auth-check to use the new "connectNow()" method after a successful login, see #25073.

comment:29 azaozz5 months ago

  • Priority changed from highest omg bbq to normal
  • Resolution set to fixed
  • Severity changed from blocker to normal
  • Status changed from new to closed

All heartbeat improvements for 3.8 are done. If there are any bugs, please open new tickets.

comment:30 nacin5 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I don't think this is done. Again: I understand there was the possibility we might break backwards compatibility here, but breaking wp.heartbeat.interval() seems a bit overboard. We should not go out of our way to break things. Clearly it was public API, as we were using it. Can we not alias it and any other renamed functions?

comment:31 nacin5 months ago

The wording around suspensions are pretty tough to parse. "wp_disable_heartbeat_suspend", $settings['suspend'] = 'disable';, etc.

How about something like$settings['allowSuspension'] = false and something like wp_heartbeat_adjust_suspension()?

comment:32 azaozz5 months ago

...breaking wp.heartbeat.interval() seems a bit overboard.

Point taken, fixing :)

How about something like $settings['allowSuspension'] = false.

Using a string gives a bit more context and can eventually be reused with $settings['suspend'] = 'enable';. Also when passing boolean through JSON, it may go through some typecasting and might end up as 0 or 1 on the JS side. That's fine as long as we do "truty/falsey" comparisons, but we are aiming to always do strict comparisons.

azaozz5 months ago

comment:33 azaozz5 months ago

In 25073-9.patch:

  • Bring back heartbeat.interval().
  • Rename wp_disable_heartbeat_suspend() to wp_heartbeat_set_suspension().
  • Rename the option for disabling suspension from options.suspend to options.suspension.

comment:34 follow-up: nacin5 months ago

I'm not necessarily objecting to setInterval, only that we should also export interval = setInterval. But if we want to just change it back, I'm fine with that too. Patch is good for me. Thanks. :-)

Boolean versus string wasn't the issue, no problems with that. 'suspend' => 'disable' was really confusing as to what was happening. 'suspension' is much better.

comment:35 azaozz5 months ago

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

In 26549:

Heartbeat:

  • Bring back heartbeat.interval().
  • Rename wp_disable_heartbeat_suspend() to wp_heartbeat_set_suspension().
  • Rename the option for disabling suspension from options.suspend to options.suspension.

Fixes #25073.

comment:36 in reply to: ↑ 34 azaozz5 months ago

I'm not necessarily objecting to setInterval, only that we should also export interval = setInterval.

Right, but I was a bit uneasy about using setInterval() too, as it is a global/built-in JS function.

Last edited 5 months ago by azaozz (previous) (diff)

comment:37 atimmer5 months ago

[26169] introduced a jshint error:

[L609:C25] W004: 'interval' is already defined.
      var interval, oldInerval = settings.tempInterval ? settings.tempInterval : settings.mainInterval;

The reason is because the function is named the same, it should be fixed by a simple rename. I have no idea what to rename it to though.

comment:38 atimmer5 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:39 nacin5 months ago

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

In 26626:

Heartbeat: Don't use a variable of the same name as the function it is in.

fixes #25073.

comment:40 DrewAPicture5 months ago

In 26732:

Inline documentation fixes for wp_heartbeat_set_suspension().

See #25073.

Note: See TracTickets for help on using tickets.