PERF: Make mega topics work without a stream.

There are tradeoffs that we took here. For the complete
story see
https://meta.discourse.org/t/performance-improvements-on-long-topics/30187/27?u=tgxworld.
This commit is contained in:
Guo Xiang Tan 2018-07-11 15:41:26 +08:00
parent de4d4747c4
commit 258e9e35ca
11 changed files with 473 additions and 69 deletions

View File

@ -618,7 +618,8 @@ export default Ember.Controller.extend(BufferedContent, {
},
selectBelow(post) {
const stream = [...this.get("model.postStream.stream")];
const postStream = this.get("model.postStream");
const stream = [...postStream.get("stream")];
const below = stream.slice(stream.indexOf(post.id));
this.get("selectedPostIds").pushObjects(below);
this.appEvents.trigger("post-stream:refresh", { force: true });
@ -884,9 +885,32 @@ export default Ember.Controller.extend(BufferedContent, {
},
_jumpToIndex(index) {
const stream = this.get("model.postStream.stream");
index = Math.max(1, Math.min(stream.length, index));
this._jumpToPostId(stream[index - 1]);
const postStream = this.get("model.postStream");
if (postStream.get("isMegaTopic")) {
this._jumpToPostNumber(index);
} else {
const stream = postStream.get("stream");
const streamIndex = Math.max(1, Math.min(stream.length, index));
this._jumpToPostId(stream[streamIndex - 1]);
}
},
_jumpToPostNumber(postNumber) {
const postStream = this.get("model.postStream");
const post = postStream.get("posts").findBy("post_number", postNumber);
if (post) {
DiscourseURL.routeTo(
this.get("model").urlForPostNumber(post.get("post_number"))
);
} else {
postStream.loadPostByPostNumber(postNumber).then(p => {
DiscourseURL.routeTo(
this.get("model").urlForPostNumber(p.get("post_number"))
);
});
}
},
_jumpToPostId(postId) {

View File

@ -50,7 +50,11 @@ export default RestModel.extend({
"stagingPost"
),
notLoading: Ember.computed.not("loading"),
filteredPostsCount: Ember.computed.alias("stream.length"),
@computed("isMegaTopic", "stream.length", "topic.highest_post_number")
filteredPostsCount(isMegaTopic, streamLength, topicHighestPostNumber) {
return isMegaTopic ? topicHighestPostNumber : streamLength;
},
@computed("posts.[]")
hasPosts() {
@ -82,8 +86,19 @@ export default RestModel.extend({
},
firstPostNotLoaded: Ember.computed.not("firstPostPresent"),
firstPostId: Ember.computed.alias("stream.firstObject"),
lastPostId: Ember.computed.alias("stream.lastObject"),
firstId: null,
lastId: null,
@computed("isMegaTopic", "stream.firstObject", "firstId")
firstPostId(isMegaTopic, streamFirstId, firstId) {
return isMegaTopic ? firstId : streamFirstId;
},
@computed("isMegaTopic", "stream.lastObject", "lastId")
lastPostId(isMegaTopic, streamLastId, lastId) {
return isMegaTopic ? lastId : streamLastId;
},
@computed("hasLoadedData", "lastPostId", "posts.@each.id")
loadedAllPosts(hasLoadedData, lastPostId) {
@ -338,23 +353,40 @@ export default RestModel.extend({
return Ember.RSVP.resolve();
}
const postIds = this.get("nextWindow");
if (Ember.isEmpty(postIds)) {
return Ember.RSVP.resolve();
}
this.set("loadingBelow", true);
const postsWithPlaceholders = this.get("postsWithPlaceholders");
postsWithPlaceholders.appending(postIds);
return this.findPostsByIds(postIds)
.then(posts => {
posts.forEach(p => this.appendPost(p));
return posts;
})
.finally(() => {
postsWithPlaceholders.finishedAppending(postIds);
if (this.get("isMegaTopic")) {
this.set("loadingBelow", true);
const fakePostIds = _.range(-1, -this.get("topic.chunk_size"), -1);
postsWithPlaceholders.appending(fakePostIds);
return this.fetchNextWindow(
this.get("posts.lastObject.post_number"),
true,
p => {
this.appendPost(p);
}
).finally(() => {
postsWithPlaceholders.finishedAppending(fakePostIds);
this.set("loadingBelow", false);
});
} else {
const postIds = this.get("nextWindow");
if (Ember.isEmpty(postIds)) return Ember.RSVP.resolve();
this.set("loadingBelow", true);
postsWithPlaceholders.appending(postIds);
return this.findPostsByIds(postIds)
.then(posts => {
posts.forEach(p => this.appendPost(p));
return posts;
})
.finally(() => {
postsWithPlaceholders.finishedAppending(postIds);
this.set("loadingBelow", false);
});
}
},
// Prepend the previous window of posts to the stream. Call it when scrolling upwards.
@ -364,21 +396,37 @@ export default RestModel.extend({
return Ember.RSVP.resolve();
}
const postIds = this.get("previousWindow");
if (Ember.isEmpty(postIds)) {
return Ember.RSVP.resolve();
}
if (this.get("isMegaTopic")) {
this.set("loadingAbove", true);
let prependedIds = [];
this.set("loadingAbove", true);
return this.findPostsByIds(postIds.reverse())
.then(posts => {
posts.forEach(p => this.prependPost(p));
})
.finally(() => {
return this.fetchNextWindow(
this.get("posts.firstObject.post_number"),
false,
p => {
this.prependPost(p);
prependedIds.push(p.get("id"));
}
).finally(() => {
const postsWithPlaceholders = this.get("postsWithPlaceholders");
postsWithPlaceholders.finishedPrepending(postIds);
postsWithPlaceholders.finishedPrepending(prependedIds);
this.set("loadingAbove", false);
});
} else {
const postIds = this.get("previousWindow");
if (Ember.isEmpty(postIds)) return Ember.RSVP.resolve();
this.set("loadingAbove", true);
return this.findPostsByIds(postIds.reverse())
.then(posts => {
posts.forEach(p => this.prependPost(p));
})
.finally(() => {
const postsWithPlaceholders = this.get("postsWithPlaceholders");
postsWithPlaceholders.finishedPrepending(postIds);
this.set("loadingAbove", false);
});
}
},
/**
@ -508,6 +556,15 @@ export default RestModel.extend({
return this._identityMap[id];
},
loadPostByPostNumber(postNumber) {
const url = `/posts/by_number/${this.get("topic.id")}/${postNumber}`;
const store = this.store;
return ajax(url).then(post => {
return this.storePost(store.createRecord("post", post));
});
},
loadPost(postId) {
const url = "/posts/" + postId;
const store = this.store;
@ -675,12 +732,19 @@ export default RestModel.extend({
// Get the index of a post in the stream. (Use this for the topic progress bar.)
progressIndexOfPost(post) {
return this.progressIndexOfPostId(post.get("id"));
return this.progressIndexOfPostId(post);
},
// Get the index in the stream of a post id. (Use this for the topic progress bar.)
progressIndexOfPostId(postId) {
return this.get("stream").indexOf(postId) + 1;
progressIndexOfPostId(post) {
const postId = post.get("id");
const index = this.get("stream").indexOf(postId);
if (this.get("isMegaTopic")) {
return post.get("post_number");
} else {
return index + 1;
}
},
/**
@ -823,6 +887,35 @@ export default RestModel.extend({
return post;
},
fetchNextWindow(postNumber, asc, callback) {
const url = `/t/${this.get("topic.id")}/posts.json`;
let data = {
post_number: postNumber,
asc: asc
};
data = _.merge(data, this.get("streamFilters"));
const store = this.store;
return ajax(url, { data }).then(result => {
if (result.suggested_topics) {
this.set("topic.suggested_topics", result.suggested_topics);
}
const posts = Ember.get(result, "post_stream.posts");
if (posts) {
posts.forEach(p => {
p = this.storePost(store.createRecord("post", p));
if (callback) {
callback.call(this, p);
}
});
}
});
},
findPostsByIds(postIds) {
const identityMap = this._identityMap;
const unloaded = postIds.filter(p => !identityMap[p]);
@ -903,6 +996,10 @@ export default RestModel.extend({
},
excerpt(streamPosition) {
if (this.get("isMegaTopic")) {
return new Ember.RSVP.Promise(resolve => resolve(""));
}
const stream = this.get("stream");
return new Ember.RSVP.Promise((resolve, reject) => {

View File

@ -44,7 +44,9 @@ export default Discourse.Route.extend({
topicController.setProperties({
"model.currentPost": closest,
enteredIndex: postStream.get("stream").indexOf(closestPost.get("id")),
enteredIndex: topic
.get("postStream")
.progressIndexOfPost(closestPost),
enteredAt: new Date().getTime().toString()
});

View File

@ -495,8 +495,9 @@ export default createWidget("topic-timeline", {
}
result.push(this.attach("timeline-controls", attrs));
const streamLength = stream.length;
if (stream.length < 3) {
if (streamLength !== 0 && streamLength < 3) {
const topicHeight = $("#topic").height();
const windowHeight = $(window).height();
if (topicHeight / windowHeight < 2.0) {

View File

@ -177,10 +177,26 @@ class TopicsController < ApplicationController
def posts
params.require(:topic_id)
params.permit(:post_ids)
params.permit(:post_ids, :post_number, :username_filters, :filter)
@topic_view = TopicView.new(params[:topic_id], current_user, post_ids: params[:post_ids])
render_json_dump(TopicViewPostsSerializer.new(@topic_view, scope: guardian, root: false, include_raw: !!params[:include_raw]))
default_options = {
filter_post_number: params[:post_number],
post_ids: params[:post_ids],
asc: ActiveRecord::Type::Boolean.new.deserialize(params[:asc]),
filter: params[:filter]
}
if (username_filters = params[:username_filters]).present?
default_options[:username_filters] = username_filters.split(',')
end
@topic_view = TopicView.new(params[:topic_id], current_user, default_options)
render_json_dump(TopicViewPostsSerializer.new(@topic_view,
scope: guardian,
root: false,
include_raw: !!params[:include_raw]
))
end
def excerpts

View File

@ -18,7 +18,16 @@ module PostStreamSerializerMixin
def post_stream
result = { posts: posts }
result[:stream] = object.filtered_post_ids if include_stream?
if include_stream?
if !object.is_mega_topic?
result[:stream] = object.filtered_post_ids
else
result[:isMegaTopic] = true
result[:firstId] = object.first_post_id
result[:lastId] = object.last_post_id
end
end
if include_gaps? && object.gaps.present?
result[:gaps] = GapSerializer.new(object.gaps, root: false)
@ -27,6 +36,10 @@ module PostStreamSerializerMixin
result
end
def include_timeline_lookup?
!object.is_mega_topic?
end
def timeline_lookup
TimelineLookup.build(object.filtered_post_stream)
end

View File

@ -4,6 +4,7 @@ require_dependency 'filter_best_posts'
require_dependency 'gaps'
class TopicView
MEGA_TOPIC_POSTS_COUNT = 25
attr_reader :topic, :posts, :guardian, :filtered_posts, :chunk_size, :print, :message_bus_last_id
attr_accessor :draft, :draft_key, :draft_sequence, :user_custom_fields, :post_custom_fields, :post_number
@ -51,7 +52,6 @@ class TopicView
self.instance_variable_set("@#{key}".to_sym, value)
end
@_post_number = @post_number.dup
@post_number = [@post_number.to_i, 1].max
@page = [@page.to_i, 1].max
@ -209,6 +209,11 @@ class TopicView
def filter_posts(opts = {})
return filter_posts_near(opts[:post_number].to_i) if opts[:post_number].present?
return filter_posts_by_ids(opts[:post_ids]) if opts[:post_ids].present?
if opts[:filter_post_number].present?
return filter_posts_by_post_number(opts[:filter_post_number], opts[:asc])
end
return filter_best(opts[:best], opts) if opts[:best].present?
filter_posts_paged(@page)
@ -244,29 +249,19 @@ class TopicView
posts_before = (@limit.to_f / 4).floor
posts_before = 1 if posts_before.zero?
sort_order_sql = <<~SQL
(
SELECT posts.sort_order
FROM posts
WHERE posts.post_number = #{post_number.to_i}
AND posts.topic_id = #{@topic.id.to_i}
LIMIT 1
)
SQL
before_post_ids = @filtered_posts.order(sort_order: :desc)
.where("posts.sort_order < #{sort_order_sql}",)
.where("posts.sort_order < #{sort_order_sql(post_number)}",)
.limit(posts_before)
.pluck(:id)
post_ids = before_post_ids + @filtered_posts.order(sort_order: :asc)
.where("posts.sort_order >= #{sort_order_sql}")
.where("posts.sort_order >= #{sort_order_sql(post_number)}")
.limit(@limit - before_post_ids.length)
.pluck(:id)
if post_ids.length < @limit
post_ids = post_ids + @filtered_posts.order(sort_order: :desc)
.where("posts.sort_order < #{sort_order_sql}")
.where("posts.sort_order < #{sort_order_sql(post_number)}")
.offset(before_post_ids.length)
.limit(@limit - post_ids.length)
.pluck(:id)
@ -456,6 +451,18 @@ class TopicView
@filtered_posts.where(post_number: post_number).pluck(:id).first
end
def is_mega_topic?
@is_mega_topic ||= (@topic.posts_count >= MEGA_TOPIC_POSTS_COUNT)
end
def first_post_id
@filtered_posts.order(sort_order: :asc).limit(1).pluck(:id).first
end
def last_post_id
@filtered_posts.order(sort_order: :desc).limit(1).pluck(:id).first
end
protected
def read_posts_set
@ -476,6 +483,18 @@ class TopicView
private
def sort_order_sql(post_number)
<<~SQL
(
SELECT posts.sort_order
FROM posts
WHERE posts.post_number = #{post_number.to_i}
AND posts.topic_id = #{@topic.id.to_i}
LIMIT 1
)
SQL
end
def filter_post_types(posts)
visible_types = Topic.visible_post_types(@user)
@ -486,6 +505,24 @@ class TopicView
end
end
def filter_posts_by_post_number(post_number, asc)
posts =
if asc
@filtered_posts
.where("sort_order > #{sort_order_sql(post_number)}")
.order(sort_order: :asc)
else
@filtered_posts
.where("sort_order < #{sort_order_sql(post_number)}")
.order(sort_order: :desc)
end
posts = posts.limit(@limit)
filter_posts_by_ids(posts.pluck(:id))
@posts = @posts.unscope(:order).order(sort_order: :desc) if !asc
end
def filter_posts_by_ids(post_ids)
# TODO: Sort might be off
@posts = Post.where(id: post_ids, topic_id: @topic.id)
@ -573,10 +610,4 @@ class TopicView
end
end
end
MEGA_TOPIC_POSTS_COUNT = 10000
def is_mega_topic?
@is_mega_topic ||= (@topic.posts_count >= MEGA_TOPIC_POSTS_COUNT)
end
end

View File

@ -328,7 +328,7 @@ describe TopicView do
end
end
context '.posts' do
context '#posts' do
# Create the posts in a different order than the sort_order
let!(:p5) { Fabricate(:post, topic: topic, user: evil_trout) }
@ -393,6 +393,44 @@ describe TopicView do
end
end
describe '#filter_posts_by_post_number' do
def create_topic_view(post_number)
TopicView.new(
topic.id,
evil_trout,
filter_post_number: post_number,
asc: asc
)
end
describe 'ascending' do
let(:asc) { true }
it 'should return the right posts' do
topic_view = create_topic_view(p3.post_number)
expect(topic_view.posts).to eq([p5])
topic_view = create_topic_view(p6.post_number)
expect(topic_view.posts).to eq([])
end
end
describe 'descending' do
let(:asc) { false }
it 'should return the right posts' do
topic_view = create_topic_view(p7.post_number)
expect(topic_view.posts).to eq([p5, p3, p2])
topic_view = create_topic_view(p2.post_number)
expect(topic_view.posts).to eq([p1])
end
end
end
describe "filter_posts_near" do
def topic_view_near(post, show_deleted = false)
@ -591,4 +629,21 @@ describe TopicView do
expect(topic_view.filtered_post_id(post.post_number)).to eq(post.id)
end
end
describe '#first_post_id and #last_post_id' do
let!(:p3) { Fabricate(:post, topic: topic) }
let!(:p2) { Fabricate(:post, topic: topic) }
let!(:p1) { Fabricate(:post, topic: topic) }
before do
[p1, p2, p3].each_with_index do |post, index|
post.update!(sort_order: index + 1)
end
end
it 'should return the right id' do
expect(topic_view.first_post_id).to eq(p1.id)
expect(topic_view.last_post_id).to eq(p3.id)
end
end
end

View File

@ -1441,12 +1441,61 @@ RSpec.describe TopicsController do
end
describe '#posts' do
let(:topic) { Fabricate(:post).topic }
let(:post) { Fabricate(:post) }
let(:topic) { post.topic }
it 'returns first posts of the topic' do
it 'returns first post of the topic' do
get "/t/#{topic.id}/posts.json"
expect(response.status).to eq(200)
expect(response.content_type).to eq('application/json')
body = JSON.parse(response.body)
expect(body["post_stream"]["posts"].first["id"]).to eq(post.id)
end
describe 'filtering by post number with filters' do
describe 'username filters' do
let!(:post2) { Fabricate(:post, topic: topic, user: Fabricate(:user)) }
let!(:post3) { Fabricate(:post, topic: topic) }
it 'should return the right posts' do
TopicView.stubs(:chunk_size).returns(2)
get "/t/#{topic.id}/posts.json", params: {
post_number: post.post_number,
username_filters: post2.user.username,
asc: true
}
expect(response.status).to eq(200)
body = JSON.parse(response.body)
expect(body["post_stream"]["posts"].first["id"]).to eq(post2.id)
end
end
describe 'summary filter' do
let!(:post2) { Fabricate(:post, topic: topic, percent_rank: 0.2) }
let!(:post3) { Fabricate(:post, topic: topic) }
it 'should return the right posts' do
TopicView.stubs(:chunk_size).returns(2)
get "/t/#{topic.id}/posts.json", params: {
post_number: post.post_number,
filter: 'summary',
asc: true
}
expect(response.status).to eq(200)
body = JSON.parse(response.body)
expect(body["post_stream"]["posts"].first["id"]).to eq(post2.id)
end
end
end
end

View File

@ -347,10 +347,28 @@ export default function() {
this.get("/t/:topic_id/posts.json", request => {
const postIds = request.queryParams.post_ids;
const posts = postIds.map(p => ({
id: parseInt(p),
post_number: parseInt(p)
}));
const postNumber = parseInt(request.queryParams.post_number);
let posts;
if (postIds) {
posts = postIds.map(p => ({
id: parseInt(p),
post_number: parseInt(p)
}));
} else if (postNumber && request.queryParams.asc === "true") {
posts = _.range(postNumber + 1, postNumber + 6).map(p => ({
id: parseInt(p),
post_number: parseInt(p)
}));
} else if (postNumber && request.queryParams.asc === "false") {
posts = _.range(postNumber - 5, postNumber)
.reverse()
.map(p => ({
id: parseInt(p),
post_number: parseInt(p)
}));
}
return response(200, { post_stream: { posts } });
});

View File

@ -484,6 +484,54 @@ QUnit.test("loadIntoIdentityMap with post ids", assert => {
});
});
QUnit.test("appendMore for megatopic", assert => {
const postStream = buildStream(1234);
const store = createStore();
const post = store.createRecord("post", { id: 1, post_number: 1 });
postStream.setProperties({
isMegaTopic: true,
posts: [post]
});
return postStream.appendMore().then(() => {
assert.present(
postStream.findLoadedPost(2),
"it adds the returned post to the store"
);
assert.equal(
postStream.get("posts").length,
6,
"it adds the right posts into the stream"
);
});
});
QUnit.test("prependMore for megatopic", assert => {
const postStream = buildStream(1234);
const store = createStore();
const post = store.createRecord("post", { id: 6, post_number: 6 });
postStream.setProperties({
isMegaTopic: true,
posts: [post]
});
return postStream.prependMore().then(() => {
assert.present(
postStream.findLoadedPost(5),
"it adds the returned post to the store"
);
assert.equal(
postStream.get("posts").length,
6,
"it adds the right posts into the stream"
);
});
});
QUnit.test("staging and undoing a new post", assert => {
const postStream = buildStream(10101, [1]);
const store = postStream.store;
@ -801,3 +849,53 @@ QUnit.test("postsWithPlaceholders", assert => {
assert.equal(testProxy.objectAt(3), p4);
});
});
QUnit.test("filteredPostsCount", assert => {
const postStream = buildStream(4567, [1, 3, 4]);
assert.equal(postStream.get("filteredPostsCount"), 3);
// Megatopic
postStream.set("isMegaTopic", true);
postStream.set("topic.highest_post_number", 4);
assert.equal(postStream.get("filteredPostsCount"), 4);
});
QUnit.test("firstPostId", assert => {
const postStream = buildStream(4567, [1, 3, 4]);
assert.equal(postStream.get("firstPostId"), 1);
postStream.setProperties({
isMegaTopic: true,
firstId: 2
});
assert.equal(postStream.get("firstPostId"), 2);
});
QUnit.test("lastPostId", assert => {
const postStream = buildStream(4567, [1, 3, 4]);
assert.equal(postStream.get("lastPostId"), 4);
postStream.setProperties({
isMegaTopic: true,
lastId: 2
});
assert.equal(postStream.get("lastPostId"), 2);
});
QUnit.test("progressIndexOfPostId", assert => {
const postStream = buildStream(4567, [1, 3, 4]);
const store = createStore();
const post = store.createRecord("post", { id: 1, post_number: 5 });
assert.equal(postStream.progressIndexOfPostId(post), 1);
postStream.set("isMegaTopic", true);
assert.equal(postStream.progressIndexOfPostId(post), 5);
});