Heartbeat performance and API improvements

[evansolomon]

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.

[johnbillion]

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.

[evansolomon]

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.

[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.

[carldanley]

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

[azaozz]

...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.

[azaozz]

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.

[evansolomon]

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.

[carldanley]

lol! +1

[azaozz]

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.

[azaozz]

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.

[nacin]

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.

[azaozz]

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.

[azaozz]

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

[azaozz]

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.

[azaozz]

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.

[azaozz]

Related: #26050

[azaozz]

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.

[TobiasBg]

Replace another $(document)

[TobiasBg]

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

[azaozz]

In 26276:

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

[azaozz]

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.

[azaozz]

In 26428:

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

[nacin]

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.

[azaozz]

In 26530:

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

[azaozz]

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

[nacin]

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?

[nacin]

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()?

[azaozz]

...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.

[azaozz]

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.

[nacin]

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.

[azaozz]

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.

[azaozz]

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.

[atimmer]

[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.

[nacin]

In 26626:

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

fixes #25073.

[DrewAPicture]

In 26732:

Inline documentation fixes for wp_heartbeat_set_suspension().

See #25073.