From 7068b58e8501c95af76ec40fa6d21e1bb7982ea8 Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Thu, 18 Apr 2019 17:30:39 +1000 Subject: [PATCH] PERF: flush topic timings less frequently This is a first step of a performance optimisation, more will follow Previously we did not properly account for previously read topics while "rushing" marking times on posts. The new mechanism now avoids "rushing" sending timings to server if all the posts were read. Also to alleviate some server load we only "ping" the server with old timings once a minute (it used to be every 20 seconds) --- .../components/scrolling-post-stream.js.es6 | 8 +- .../discourse/lib/screen-track.js.es6 | 27 +++++-- config/site_settings.yml | 2 +- test/javascripts/lib/screen-track-test.js.es6 | 75 +++++++++++++++++++ 4 files changed, 102 insertions(+), 10 deletions(-) create mode 100644 test/javascripts/lib/screen-track-test.js.es6 diff --git a/app/assets/javascripts/discourse/components/scrolling-post-stream.js.es6 b/app/assets/javascripts/discourse/components/scrolling-post-stream.js.es6 index db395dd2ee9..a1f101a8dc0 100644 --- a/app/assets/javascripts/discourse/components/scrolling-post-stream.js.es6 +++ b/app/assets/javascripts/discourse/components/scrolling-post-stream.js.es6 @@ -236,15 +236,21 @@ export default MountWidget.extend({ } const onscreenPostNumbers = []; + const readPostNumbers = []; + const prev = this._previouslyNearby; const newPrev = {}; nearby.forEach(idx => { const post = posts.objectAt(idx); const postNumber = post.post_number; + delete prev[postNumber]; if (onscreen.indexOf(idx) !== -1) { onscreenPostNumbers.push(postNumber); + if (post.read) { + readPostNumbers.push(postNumber); + } } newPrev[postNumber] = post; uncloak(post, this); @@ -253,7 +259,7 @@ export default MountWidget.extend({ Object.values(prev).forEach(node => cloak(node, this)); this._previouslyNearby = newPrev; - this.screenTrack.setOnscreen(onscreenPostNumbers); + this.screenTrack.setOnscreen(onscreenPostNumbers, readPostNumbers); }, _scrollTriggered() { diff --git a/app/assets/javascripts/discourse/lib/screen-track.js.es6 b/app/assets/javascripts/discourse/lib/screen-track.js.es6 index 58982a1829e..3050de3a1d5 100644 --- a/app/assets/javascripts/discourse/lib/screen-track.js.es6 +++ b/app/assets/javascripts/discourse/lib/screen-track.js.es6 @@ -54,8 +54,9 @@ export default class { } } - setOnscreen(onscreen) { + setOnscreen(onscreen, readOnscreen) { this._onscreen = onscreen; + this._readOnscreen = readOnscreen; } // Reset our timers @@ -68,6 +69,8 @@ export default class { this._totalTimings = {}; this._topicTime = 0; this._onscreen = []; + this._readOnscreen = []; + this._readPosts = {}; this._inProgress = false; } @@ -112,6 +115,7 @@ export default class { if (!$.isEmptyObject(newTimings)) { if (this.currentUser) { this._inProgress = true; + ajax("/topics/timings", { data: { timings: newTimings, @@ -198,20 +202,27 @@ export default class { const nextFlush = this.siteSettings.flush_timings_secs * 1000; const rush = Object.keys(timings).some(postNumber => { - return timings[postNumber] > 0 && !totalTimings[postNumber]; + return ( + timings[postNumber] > 0 && + !totalTimings[postNumber] && + !this._readPosts[postNumber] + ); }); if (!this._inProgress && (this._lastFlush > nextFlush || rush)) { this.flush(); } - // Don't track timings if we're not in focus - if (!Discourse.get("hasFocus")) return; + if (Discourse.get("hasFocus")) { + this._topicTime += diff; - this._topicTime += diff; + this._onscreen.forEach( + postNumber => (timings[postNumber] = (timings[postNumber] || 0) + diff) + ); - this._onscreen.forEach( - postNumber => (timings[postNumber] = (timings[postNumber] || 0) + diff) - ); + this._readOnscreen.forEach(postNumber => { + this._readPosts[postNumber] = true; + }); + } } } diff --git a/config/site_settings.yml b/config/site_settings.yml index 86ae8b3c31d..ac714ca5a0f 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1443,7 +1443,7 @@ developer: max: 99000 flush_timings_secs: client: true - default: 20 + default: 60 active_user_rate_limit_secs: 60 verbose_localization: default: false diff --git a/test/javascripts/lib/screen-track-test.js.es6 b/test/javascripts/lib/screen-track-test.js.es6 new file mode 100644 index 00000000000..b356dc9b9f0 --- /dev/null +++ b/test/javascripts/lib/screen-track-test.js.es6 @@ -0,0 +1,75 @@ +import TopicTrackingState from "discourse/models/topic-tracking-state"; +import Session from "discourse/models/session"; +import ScreenTrack from "discourse/lib/screen-track"; + +let clock; + +QUnit.module("lib:screen-track", { + beforeEach() { + clock = sinon.useFakeTimers(new Date(2012, 11, 31, 12, 0).getTime()); + }, + + afterEach() { + clock.restore(); + } +}); + +QUnit.test("Correctly flushes posts as needed", assert => { + const timings = []; + + // prettier-ignore + server.post("/topics/timings", t => { //eslint-disable-line + timings.push(t); + return [200, {}, ""]; + }); + + const trackingState = TopicTrackingState.create(); + const siteSettings = { + flush_timings_secs: 60 + }; + + const currentUser = { id: 1, username: "sam" }; + + const tracker = new ScreenTrack( + trackingState, + siteSettings, + Session.current(), + currentUser + ); + + const topicController = null; + + Discourse.set("hasFocus", true); + + tracker.reset(); + tracker.start(1, topicController); + + tracker.setOnscreen([1, 2, 3], [1, 2, 3]); + + clock.tick(1050); + clock.tick(1050); + + // no ajax yet + assert.equal(0, timings.length); + + tracker.setOnscreen([1, 2, 3, 4], [1, 2, 3]); + + clock.tick(1050); + clock.tick(1050); + + // we should be rushed now cause there is a new thing on the screen + assert.equal(1, timings.length); + + const req = + "timings%5B1%5D=3000&timings%5B2%5D=3000&timings%5B3%5D=3000&timings%5B4%5D=1000&topic_time=3000&topic_id=1"; + assert.equal(timings[0].requestBody, req); + + tracker.stop(); + + assert.equal(2, timings.length); + + const req2 = + "timings%5B1%5D=1200&timings%5B2%5D=1200&timings%5B3%5D=1200&timings%5B4%5D=1200&topic_time=1200&topic_id=1"; + + assert.equal(timings[1].requestBody, req2); +});