PERF: Move mention lookups out of the V8 context. (#6640)

PERF: Move mention lookups out of the V8 context. (#6640)

We were looking up each mention one by one without any form of caching and that results
in a problem somewhat similar to an N+1. When we have to do alot of DB
lookups, it also increased the time spent in the V8 context which may
eventually lead to a timeout. The change here makes it such that mention lookups only does a single
DB query per post that happens outside of the V8 context.

From c5a70eca6e7415d508d097ff90b69ad5d15a94b7 Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <gxtan1990@gmail.com>
Date: Thu, 22 Nov 2018 14:28:48 +0800
Subject: [PATCH] PERF: Move mention lookups out of the V8 context. (#6640)

We were looking up each mention one by one without any form of caching and that results
in a problem somewhat similar to an N+1. When we have to do alot of DB
lookups, it also increased the time spent in the V8 context which may
eventually lead to a timeout. The change here makes it such that mention lookups only does a single
DB query per post that happens outside of the V8 context.

diff --git a/app/assets/javascripts/pretty-text/engines/discourse-markdown/mentions.js.es6 b/app/assets/javascripts/pretty-text/engines/discourse-markdown/mentions.js.es6
index 97cca51..d35500b 100644
--- a/app/assets/javascripts/pretty-text/engines/discourse-markdown/mentions.js.es6
+++ b/app/assets/javascripts/pretty-text/engines/discourse-markdown/mentions.js.es6
@@ -1,32 +1,12 @@
 function addMention(buffer, matches, state) {
   let username = matches[1] || matches[2];
-  let { getURL, mentionLookup, formatUsername } = state.md.options.discourse;
-
-  let type = mentionLookup && mentionLookup(username);
-
-  let tag = "a";
+  let tag = "span";
   let className = "mention";
-  let href = null;
-
-  if (type === "user") {
-    href = getURL("/u/") + username.toLowerCase();
-  } else if (type === "group") {
-    href = getURL("/groups/") + username;
-    className = "mention-group";
-  } else {
-    tag = "span";
-  }
 
   let token = new state.Token("mention_open", tag, 1);
   token.attrs = [["class", className]];
-  if (href) {
-    token.attrs.push(["href", href]);
-  }
 
   buffer.push(token);
-  if (formatUsername) {
-    username = formatUsername(username);
-  }
 
   token = new state.Token("text", "", 0);
   token.content = "@" + username;
diff --git a/app/assets/javascripts/pretty-text/pretty-text.js.es6 b/app/assets/javascripts/pretty-text/pretty-text.js.es6
index 7050e38..410a47e 100644
--- a/app/assets/javascripts/pretty-text/pretty-text.js.es6
+++ b/app/assets/javascripts/pretty-text/pretty-text.js.es6
@@ -31,7 +31,6 @@ export function buildOptions(state) {
     previewing,
     linkify,
     censoredWords,
-    mentionLookup,
     invalidateOneboxes
   } = state;
 
@@ -67,7 +66,6 @@ export function buildOptions(state) {
     lookupAvatarByPostNumber,
     lookupPrimaryUserGroupByPostNumber,
     formatUsername,
-    mentionLookup,
     emojiUnicodeReplacer,
     lookupInlineOnebox,
     lookupImageUrls,
diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb
index f43dcdb..d30a4f8 100644
--- a/lib/pretty_text.rb
+++ b/lib/pretty_text.rb
@@ -156,7 +156,6 @@ module PrettyText
         __optInput.formatUsername = __formatUsername;
         __optInput.getTopicInfo = __getTopicInfo;
         __optInput.categoryHashtagLookup = __categoryLookup;
-        __optInput.mentionLookup = __mentionLookup;
         __optInput.customEmoji = #{custom_emoji.to_json};
         __optInput.emojiUnicodeReplacer = __emojiUnicodeReplacer;
         __optInput.lookupInlineOnebox = __lookupInlineOnebox;
@@ -265,6 +264,8 @@ module PrettyText
       add_s3_cdn(doc)
     end
 
+    add_mentions(doc) if SiteSetting.enable_mentions
+
     doc.to_html
   end
 
@@ -419,4 +420,67 @@ module PrettyText
     end
   end
 
+  private
+
+  USER_TYPE ||= 'user'
+  GROUP_TYPE ||= 'group'
+
+  def self.add_mentions(doc)
+    elements = doc.css("span.mention")
+    names = elements.map { |element| element.text[1..-1] }
+
+    mentions = lookup_mentions(names)
+
+    doc.css("span.mention").each do |element|
+      name = element.text[1..-1]
+      name.downcase!
+
+      if type = mentions[name]
+        element.name = 'a'
+
+        element.children = PrettyText::Helpers.format_username(
+          element.children.text
+        )
+
+        case type
+        when USER_TYPE
+          element['href'] = "/u/#{name}"
+        when GROUP_TYPE
+          element['class'] = 'mention-group'
+          element['href'] = "/groups/#{name}"
+        end
+      end
+    end
+  end
+
+  def self.lookup_mentions(names)
+    sql = <<~SQL
+    (
+      SELECT
+        :user_type AS type,
+        username_lower AS name
+      FROM users
+      WHERE username_lower IN (:names)
+    )
+    UNION
+    (
+      SELECT
+        :group_type AS type,
+        name
+      FROM groups
+      WHERE name IN (:names)
+    )
+    SQL
+
+    results = DB.query(sql,
+      names: names,
+      user_type: USER_TYPE,
+      group_type: GROUP_TYPE
+    )
+
+    mentions = {}
+    results.each { |result| mentions[result.name] = result.type }
+    mentions
+  end
+
 end
diff --git a/lib/pretty_text/helpers.rb b/lib/pretty_text/helpers.rb
index 54b33a2..05df0cc 100644
--- a/lib/pretty_text/helpers.rb
+++ b/lib/pretty_text/helpers.rb
@@ -39,12 +39,6 @@ module PrettyText
       username
     end
 
-    def mention_lookup(name)
-      return false   if name.blank?
-      return "user"  if User.exists?(username_lower: name.downcase)
-      return "group" if Group.exists?(name: name)
-    end
-
     def category_hashtag_lookup(category_slug)
       if category = Category.query_from_hashtag_slug(category_slug)
         [category.url_with_id, category_slug]
diff --git a/lib/pretty_text/shims.js b/lib/pretty_text/shims.js
index 6801779..b2ce2cd 100644
--- a/lib/pretty_text/shims.js
+++ b/lib/pretty_text/shims.js
@@ -1,20 +1,26 @@
-__PrettyText = require('pretty-text/pretty-text').default;
-__buildOptions = require('pretty-text/pretty-text').buildOptions;
-__performEmojiUnescape = require('pretty-text/emoji').performEmojiUnescape;
+__PrettyText = require("pretty-text/pretty-text").default;
+__buildOptions = require("pretty-text/pretty-text").buildOptions;
+__performEmojiUnescape = require("pretty-text/emoji").performEmojiUnescape;
 
-__utils = require('discourse/lib/utilities');
+__utils = require("discourse/lib/utilities");
 
 __emojiUnicodeReplacer = null;
 
 __setUnicode = function(replacements) {
-  let unicodeRegexp = new RegExp(Object.keys(replacements).sort().reverse().join("|"), "g");
+  let unicodeRegexp = new RegExp(
+    Object.keys(replacements)
+      .sort()
+      .reverse()
+      .join("|"),
+    "g"
+  );
 
   __emojiUnicodeReplacer = function(text) {
     unicodeRegexp.lastIndex = 0;
     let m;
     while ((m = unicodeRegexp.exec(text)) !== null) {
       let replacement = ":" + replacements[m[0]] + ":";
-      const before = text.charAt(m.index-1);
+      const before = text.charAt(m.index - 1);
       if (!/\B/.test(before)) {
         replacement = "\u200b" + replacement;
       }
@@ -23,7 +29,7 @@ __setUnicode = function(replacements) {
 
     // fixes Safari VARIATION SELECTOR-16 issue with some emojis
     // https://meta.discourse.org/t/emojis-selected-on-ios-displaying-additional-rectangles/86132
-    text = text.replace(/\ufe0f/g, '');
+    text = text.replace(/\ufe0f/g, "");
 
     return text;
   };
@@ -35,9 +41,13 @@ function __getURLNoCDN(url) {
   if (!url) return url;
 
   // if it's a non relative URL, return it.
-  if (url !== '/' && !/^\/[^\/]/.test(url)) { return url; }
+  if (url !== "/" && !/^\/[^\/]/.test(url)) {
+    return url;
+  }
 
-  if (url.indexOf(__paths.baseUri) !== -1) { return url; }
+  if (url.indexOf(__paths.baseUri) !== -1) {
+    return url;
+  }
   if (url[0] !== "/") url = "/" + url;
 
   return __paths.baseUri + url;
@@ -76,12 +86,11 @@ function __categoryLookup(c) {
   return __helpers.category_tag_hashtag_lookup(c);
 }
 
-function __mentionLookup(u) {
-  return __helpers.mention_lookup(u);
-}
-
 function __lookupAvatar(p) {
-  return __utils.avatarImg({size: "tiny", avatarTemplate: __helpers.avatar_template(p) }, __getURL);
+  return __utils.avatarImg(
+    { size: "tiny", avatarTemplate: __helpers.avatar_template(p) },
+    __getURL
+  );
 }
 
 function __formatUsername(username) {
@@ -97,5 +106,7 @@ function __getCurrentUser(userId) {
 }
 
 I18n = {
-  t: fun

GitHub

You know the problem though is there is a bit of discord between what the client finds out with is_local_username and what the server later on see. I think we really should strip out unmentionable Groups and staged users, surprised we never picked up on this bug earlier, should also add some tests for it.

2 Likes

O good catch :+1: Let me see if I can unify the queries here.

@SamSaffron Done in

and

2 Likes