From 5a8bccfe08ffe1a79144c88e50f64cf1abb5a019 Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Mon, 24 Jul 2017 16:16:13 -0400 Subject: [PATCH] topic_data.js: Refactor topic history internals. This commit introduces a per-stream topic_history class inside of topic_data.js to better encapsulate how we store topic history. To the callers, nothing changes here. (Some of our non-black-box node tests change their way of setting up data, though, since the internal data structures are different.) The new class has the following improvements: * We use message_id instead of timestamp as our sorting key. (We could have done this in a prep commit, but it wouldn't have made the diff much cleaner here.) * We use a dictionary instead of a sorted list to store the data, so that writes are O(1) instead of O(NlogN). Reads now do sorts, so they're O(NlogN) instead of O(N), but reads are fairly infrequent. (The main goal here isn't actually performance, but instead it just simplifies the implementation.) * We isolate `topic_history` from the format of the messages. This prepares us for upcoming changes where updates to the data structure may come from topic history queries as well as messages. * We split out the message-add path from the message-remove path. This prepares us to eventually get rid of the "count" mechanism that is kind of fragile and which has to be bypassed for historical topics. --- .../node_tests/search_suggestion.js | 39 ++--- frontend_tests/node_tests/stream_data.js | 40 +++-- frontend_tests/node_tests/topic_list.js | 12 +- static/js/topic_data.js | 144 ++++++++++++++---- 4 files changed, 162 insertions(+), 73 deletions(-) diff --git a/frontend_tests/node_tests/search_suggestion.js b/frontend_tests/node_tests/search_suggestion.js index 83eb717d34..b9779e360c 100644 --- a/frontend_tests/node_tests/search_suggestion.js +++ b/frontend_tests/node_tests/search_suggestion.js @@ -36,7 +36,7 @@ init(); set_global('narrow', {}); -topic_data.populate_for_tests({}); +topic_data.reset(); (function test_basic_get_suggestions() { var query = 'fred'; @@ -575,25 +575,24 @@ init(); } }; - var recent_data = {}; - topic_data.populate_for_tests(recent_data); + topic_data.reset(); suggestions = search.get_suggestions('te'); expected = [ "te", ]; assert.deepEqual(suggestions.strings, expected); - recent_data[devel_id] = [ - {name: 'REXX'}, - ]; + topic_data.add_message({ + stream_id: devel_id, + topic_name: 'REXX', + }); - recent_data[office_id] = [ - {name: 'team'}, - {name: 'ignore'}, - {name: 'test'}, - ]; - - topic_data.populate_for_tests(recent_data); + _.each(['team', 'ignore', 'test'], function (topic_name) { + topic_data.add_message({ + stream_id: office_id, + topic_name: topic_name, + }); + }); suggestions = search.get_suggestions('te'); expected = [ @@ -678,7 +677,7 @@ init(); return; }; - topic_data.populate_for_tests({}); + topic_data.reset(); var suggestions = search.get_suggestions(query); @@ -698,7 +697,7 @@ init(); return; }; - topic_data.populate_for_tests({}); + topic_data.reset(); var query = 'stream:of'; var suggestions = search.get_suggestions(query); @@ -750,13 +749,7 @@ init(); people.add(alice); - topic_data.populate_for_tests({ - office: [ - {name: 'team'}, - {name: 'ignore'}, - {name: 'test'}, - ], - }); + topic_data.reset(); var suggestions = search.get_suggestions(query); @@ -882,7 +875,7 @@ init(); return; }; - topic_data.populate_for_tests({}); + topic_data.reset(); // test allowing spaces with quotes surrounding operand var query = 'stream:"dev he"'; diff --git a/frontend_tests/node_tests/stream_data.js b/frontend_tests/node_tests/stream_data.js index ddbf382f15..cafa98b2ce 100644 --- a/frontend_tests/node_tests/stream_data.js +++ b/frontend_tests/node_tests/stream_data.js @@ -252,37 +252,53 @@ var people = global.people; stream_data.add_sub('Rome', rome); - var message = { + var message1 = { stream_id: stream_id, - timestamp: 101, + id: 101, subject: 'toPic1', }; - topic_data.process_message(message); + topic_data.process_message(message1); var history = topic_data.get_recent_names(rome.stream_id); assert.deepEqual(history, ['toPic1']); - message = { + var message2 = { stream_id: stream_id, - timestamp: 102, + id: 102, subject: 'Topic1', }; - topic_data.process_message(message); + topic_data.process_message(message2); history = topic_data.get_recent_names(rome.stream_id); assert.deepEqual(history, ['Topic1']); - message = { + var message3 = { stream_id: stream_id, - timestamp: 103, + id: 103, subject: 'topic2', }; - topic_data.process_message(message); + topic_data.process_message(message3); history = topic_data.get_recent_names(rome.stream_id); assert.deepEqual(history, ['topic2', 'Topic1']); - topic_data.process_message(message, true); + // Removing first topic1 message has no effect. + topic_data.process_message(message1, true); history = topic_data.get_recent_names(rome.stream_id); - assert.deepEqual(history, ['Topic1']); + assert.deepEqual(history, ['topic2', 'Topic1']); + + // Remove second topic1 message removes the topic. + topic_data.process_message(message2, true); + history = topic_data.get_recent_names(rome.stream_id); + assert.deepEqual(history, ['topic2']); + + // Test that duplicate remove does not crash us. + topic_data.process_message(message2, true); + history = topic_data.get_recent_names(rome.stream_id); + assert.deepEqual(history, ['topic2']); + + // get to 100% coverage for defensive code + topic_data.remove_message({ + stream_id: 9999999, + }); }()); (function test_is_active() { @@ -306,7 +322,7 @@ var people = global.people; var message = { stream_id: 222, - timestamp: 108, + id: 108, subject: 'topic2', }; topic_data.process_message(message); diff --git a/frontend_tests/node_tests/topic_list.js b/frontend_tests/node_tests/topic_list.js index 84ef81139c..51ef658ed1 100644 --- a/frontend_tests/node_tests/topic_list.js +++ b/frontend_tests/node_tests/topic_list.js @@ -24,11 +24,13 @@ global.compile_template('topic_list_item'); var active_topic = "testing"; var max_topics = 5; - var recent_topics = {}; - recent_topics[stream_id] = [ - {name: "coding"}, - ]; - topic_data.populate_for_tests(recent_topics); + topic_data.reset(); + topic_data.add_message({ + stream_id: stream_id, + topic_name: 'coding', + message_id: 400, + }); + global.unread.num_unread_for_subject = function () { return 1; }; diff --git a/static/js/topic_data.js b/static/js/topic_data.js index a95fab658a..9ab62649cf 100644 --- a/static/js/topic_data.js +++ b/static/js/topic_data.js @@ -8,58 +8,136 @@ exports.stream_has_topics = function (stream_id) { return stream_dict.has(stream_id); }; -exports.process_message = function (message, remove_message) { - var current_timestamp = 0; - var count = 0; - var stream_id = message.stream_id; - var canon_topic = message.subject.toLowerCase(); +exports.topic_history = function () { + var topics = new Dict({fold_case: true}); - var recents = stream_dict.get(stream_id) || []; + var self = {}; - recents = _.filter(recents, function (item) { - var is_duplicate = ( - item.name.toLowerCase() === canon_topic); - if (is_duplicate) { - current_timestamp = item.timestamp; - count = item.count; + self.add_or_update = function (opts) { + var name = opts.name; + var message_id = opts.message_id || 0; + + message_id = parseInt(message_id, 10); + + var existing = topics.get(name); + + if (!existing) { + topics.set(opts.name, { + message_id: message_id, + pretty_name: name, + count: 1, + }); + return; } - return !is_duplicate; - }); - if (remove_message !== undefined) { - count = count - 1; - } else { - count = count + 1; + existing.count += 1; + if (message_id > existing.message_id) { + existing.message_id = message_id; + existing.pretty_name = name; + } + }; + + self.maybe_remove = function (topic_name) { + var existing = topics.get(topic_name); + + if (!existing) { + return; + } + + if (existing.count <= 1) { + topics.del(topic_name); + return; + } + + existing.count -= 1; + }; + + self.get_recent_names = function () { + var recents = topics.values(); + + recents.sort(function (a, b) { + return b.message_id - a.message_id; + }); + + var names = _.map(recents, function (obj) { + return obj.pretty_name; + }); + + return names; + }; + + return self; +}; + +exports.process_message = function (message, remove_message) { + if (remove_message) { + exports.remove_message({ + stream_id: message.stream_id, + topic_name: message.subject, + }); + return; } - if (count !== 0) { - recents.push({name: message.subject, - count: count, - timestamp: Math.max(message.timestamp, current_timestamp)}); + exports.add_message({ + stream_id: message.stream_id, + topic_name: message.subject, + message_id: message.id, + }); +}; + +exports.remove_message = function (opts) { + var stream_id = opts.stream_id; + var name = opts.topic_name; + var history = stream_dict.get(stream_id); + + // This is the special case of "removing" a message from + // a topic, which happens when we edit topics. + + if (!history) { + return; } - recents.sort(function (a, b) { - return b.timestamp - a.timestamp; - }); + // This is the normal case of an incoming message. + history.maybe_remove(name); +}; - stream_dict.set(stream_id, recents); +exports.find_or_create = function (stream_id) { + var history = stream_dict.get(stream_id); + + if (!history) { + history = exports.topic_history(); + stream_dict.set(stream_id, history); + } + + return history; +}; + +exports.add_message = function (opts) { + var stream_id = opts.stream_id; + var message_id = opts.message_id; + var name = opts.topic_name; + + var history = exports.find_or_create(stream_id); + + history.add_or_update({ + name: name, + message_id: message_id, + }); }; exports.get_recent_names = function (stream_id) { - var topic_objs = stream_dict.get(stream_id); + var history = stream_dict.get(stream_id); - if (!topic_objs) { + if (!history) { return []; } - return _.map(topic_objs, function (obj) { - return obj.name; - }); + return history.get_recent_names(); }; -exports.populate_for_tests = function (stream_map) { +exports.reset = function () { // This is only used by tests. - stream_dict = Dict.from(stream_map); + stream_dict = new Dict(); }; return exports;