WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years 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 7 years ago.
25073.2.patch (4.8 KB) - added by azaozz 7 years ago.
25073.3.patch (23.0 KB) - added by azaozz 7 years ago.
25073-4.patch (27.7 KB) - added by azaozz 7 years ago.
25073-5.patch (28.7 KB) - added by azaozz 7 years ago.
25073-6.patch (28.3 KB) - added by azaozz 7 years ago.
25073-7.patch (1.9 KB) - added by azaozz 7 years ago.
25073-cache-document.patch (460 bytes) - added by TobiasBg 7 years ago.
Replace another $(document)
25073-8.patch (3.7 KB) - added by azaozz 7 years ago.
25073-9.patch (3.0 KB) - added by azaozz 7 years ago.

Download all attachments as: .zip

Change History (50)

@evansolomon
7 years ago

#1 follow-up: @johnbillion
7 years 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.

#2 in reply to: ↑ 1 @evansolomon
7 years 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.

#3 @azaozz
7 years ago

  • Cc carldanley added

#4 follow-up: @carldanley
7 years 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.

#5 @carldanley
7 years ago

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

#6 @azaozz
7 years 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.

#7 @kadamwhite
7 years ago

  • Cc kadamwhite@… added

#8 @azaozz
7 years 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.

#9 @azaozz
7 years ago

  • Cc kadamwhite@… added

#10 in reply to: ↑ 4 @evansolomon
7 years 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

#11 @carldanley
7 years ago

lol! +1

@azaozz
7 years ago

#12 @azaozz
7 years 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.

@azaozz
7 years ago

#13 @azaozz
7 years 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.

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

@azaozz
7 years ago

#15 @azaozz
7 years 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.

@azaozz
7 years ago

#16 @azaozz
7 years ago

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

#17 @DJPaul
7 years ago

  • Cc djpaul@… added

#18 @adamsilverstein
7 years ago

  • Cc adamsilverstein@… added

@azaozz
7 years ago

#19 @azaozz
7 years 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.

#20 @azaozz
7 years 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.

#21 @azaozz
7 years ago

Related: #26050

@azaozz
7 years ago

#22 @azaozz
7 years 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.

@TobiasBg
7 years ago

Replace another $(document)

#23 @TobiasBg
7 years ago

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

#24 @azaozz
7 years ago

In 26276:

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

@azaozz
7 years ago

#25 @azaozz
7 years 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.

#26 @azaozz
7 years ago

In 26428:

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

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

#28 @azaozz
7 years ago

In 26530:

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

#29 @azaozz
7 years 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.

#30 @nacin
7 years 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?

#31 @nacin
7 years 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()?

#32 @azaozz
7 years 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.

@azaozz
7 years ago

#33 @azaozz
7 years 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.

#34 follow-up: @nacin
7 years 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.

#35 @azaozz
7 years 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.

#36 in reply to: ↑ 34 @azaozz
7 years 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 7 years ago by azaozz (previous) (diff)

#37 @atimmer
7 years 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.

#38 @atimmer
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#40 @DrewAPicture
7 years ago

In 26732:

Inline documentation fixes for wp_heartbeat_set_suspension().

See #25073.

Note: See TracTickets for help on using tickets.