PERF: Move processing of inline onebox out of V8 context. (#6658)

PERF: Move processing of inline onebox out of V8 context. (#6658)

From 57e2f4990d4a422c8ad38b64ed91b53fc92161bc Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <gxtan1990@gmail.com>
Date: Mon, 26 Nov 2018 09:21:38 +0800
Subject: [PATCH] PERF: Move processing of inline onebox out of V8 context.
 (#6658)


diff --git a/app/assets/javascripts/discourse/components/composer-editor.js.es6 b/app/assets/javascripts/discourse/components/composer-editor.js.es6
index ce9b6a5..090e757 100644
--- a/app/assets/javascripts/discourse/components/composer-editor.js.es6
+++ b/app/assets/javascripts/discourse/components/composer-editor.js.es6
@@ -36,6 +36,8 @@ import {
   resolveAllShortUrls
 } from "pretty-text/image-short-url";
 
+import { INLINE_ONEBOX_LOADING_CSS_CLASS } from "pretty-text/inline-oneboxer";
+
 const REBUILD_SCROLL_MAP_EVENTS = ["composer:resized", "composer:typed-reply"];
 
 const uploadHandlers = [];
@@ -993,10 +995,10 @@ export default Ember.Component.extend({
       resolveAllShortUrls(ajax);
 
       let inline = {};
-      $("a.inline-onebox-loading", $preview).each(function(index, link) {
-        let $link = $(link);
-        $link.removeClass("inline-onebox-loading");
-        let text = $link.text();
+      $(`a.${INLINE_ONEBOX_LOADING_CSS_CLASS}`, $preview).each((index, link) => {
+        const $link = $(link);
+        $link.removeClass(INLINE_ONEBOX_LOADING_CSS_CLASS);
+        const text = $link.text();
         inline[text] = inline[text] || [];
         inline[text].push($link);
       });
diff --git a/app/assets/javascripts/pretty-text/engines/discourse-markdown/onebox.js.es6 b/app/assets/javascripts/pretty-text/engines/discourse-markdown/onebox.js.es6
index 8306026..f437988 100644
--- a/app/assets/javascripts/pretty-text/engines/discourse-markdown/onebox.js.es6
+++ b/app/assets/javascripts/pretty-text/engines/discourse-markdown/onebox.js.es6
@@ -1,5 +1,9 @@
 import { lookupCache } from "pretty-text/oneboxer";
-import { cachedInlineOnebox } from "pretty-text/inline-oneboxer";
+
+import {
+  cachedInlineOnebox,
+  INLINE_ONEBOX_LOADING_CSS_CLASS
+} from "pretty-text/inline-oneboxer";
 
 const ONEBOX = 1;
 const INLINE = 2;
@@ -94,23 +98,13 @@ function applyOnebox(state, silent) {
               attrs.push(["class", "onebox"]);
               attrs.push(["target", "_blank"]);
             }
-          } else if (mode === INLINE) {
-            if (!isTopLevel(href)) {
-              let onebox = cachedInlineOnebox(href);
-              let options = state.md.options.discourse;
-
-              if (options.lookupInlineOnebox) {
-                onebox = options.lookupInlineOnebox(
-                  href,
-                  options.invalidateOneboxes
-                );
-              }
-
-              if (onebox && onebox.title) {
-                text.content = onebox.title;
-              } else if (state.md.options.discourse.previewing && !onebox) {
-                attrs.push(["class", "inline-onebox-loading"]);
-              }
+          } else if (mode === INLINE && !isTopLevel(href)) {
+            const onebox = cachedInlineOnebox(href);
+
+            if (onebox && onebox.title) {
+              text.content = onebox.title;
+            } else {
+              attrs.push(["class", INLINE_ONEBOX_LOADING_CSS_CLASS]);
             }
           }
         }
diff --git a/app/assets/javascripts/pretty-text/inline-oneboxer.js.es6 b/app/assets/javascripts/pretty-text/inline-oneboxer.js.es6
deleted file mode 100644
index a7b6a99..0000000
--- a/app/assets/javascripts/pretty-text/inline-oneboxer.js.es6
+++ /dev/null
@@ -1,26 +0,0 @@
-let _cache = {};
-
-export function applyInlineOneboxes(inline, ajax) {
-  Object.keys(inline).forEach(url => {
-    // cache a blank locally, so we never trigger a lookup
-    _cache[url] = {};
-  });
-
-  return ajax("/inline-onebox", {
-    data: { urls: Object.keys(inline) }
-  }).then(result => {
-    result["inline-oneboxes"].forEach(onebox => {
-      if (onebox.title) {
-        _cache[onebox.url] = onebox;
-        let links = inline[onebox.url] || [];
-        links.forEach(link => {
-          link.text(onebox.title);
-        });
-      }
-    });
-  });
-}
-
-export function cachedInlineOnebox(url) {
-  return _cache[url];
-}
diff --git a/app/assets/javascripts/pretty-text/inline-oneboxer.js.es6.erb b/app/assets/javascripts/pretty-text/inline-oneboxer.js.es6.erb
new file mode 100644
index 0000000..983549d
--- /dev/null
+++ b/app/assets/javascripts/pretty-text/inline-oneboxer.js.es6.erb
@@ -0,0 +1,29 @@
+let _cache = {};
+
+export const INLINE_ONEBOX_LOADING_CSS_CLASS =
+  "<%= CookedPostProcessor::INLINE_ONEBOX_LOADING_CSS_CLASS %>";
+
+export function applyInlineOneboxes(inline, ajax) {
+  Object.keys(inline).forEach(url => {
+    // cache a blank locally, so we never trigger a lookup
+    _cache[url] = {};
+  });
+
+  return ajax("/inline-onebox", {
+    data: { urls: Object.keys(inline) }
+  }).then(result => {
+    result["inline-oneboxes"].forEach(onebox => {
+      if (onebox.title) {
+        _cache[onebox.url] = onebox;
+        let links = inline[onebox.url] || [];
+        links.forEach(link => {
+          link.text(onebox.title);
+        });
+      }
+    });
+  });
+}
+
+export function cachedInlineOnebox(url) {
+  return _cache[url];
+}
diff --git a/app/assets/javascripts/pretty-text/pretty-text.js.es6 b/app/assets/javascripts/pretty-text/pretty-text.js.es6
index 410a47e..da572d8 100644
--- a/app/assets/javascripts/pretty-text/pretty-text.js.es6
+++ b/app/assets/javascripts/pretty-text/pretty-text.js.es6
@@ -26,12 +26,10 @@ export function buildOptions(state) {
     lookupPrimaryUserGroupByPostNumber,
     formatUsername,
     emojiUnicodeReplacer,
-    lookupInlineOnebox,
     lookupImageUrls,
     previewing,
     linkify,
-    censoredWords,
-    invalidateOneboxes
+    censoredWords
   } = state;
 
   let features = {
@@ -67,7 +65,6 @@ export function buildOptions(state) {
     lookupPrimaryUserGroupByPostNumber,
     formatUsername,
     emojiUnicodeReplacer,
-    lookupInlineOnebox,
     lookupImageUrls,
     censoredWords,
     allowedHrefSchemes: siteSettings.allowed_href_schemes
@@ -79,8 +76,7 @@ export function buildOptions(state) {
     markdownIt: true,
     injectLineNumbersToPreview:
       siteSettings.enable_advanced_editor_preview_sync,
-    previewing,
-    invalidateOneboxes
+    previewing
   };
 
   // note, this will mutate options due to the way the API is designed
diff --git a/app/assets/javascripts/pretty-text/white-lister.js.es6 b/app/assets/javascripts/pretty-text/white-lister.js.es6
index 0ec0039..4e1ed92 100644
--- a/app/assets/javascripts/pretty-text/white-lister.js.es6
+++ b/app/assets/javascripts/pretty-text/white-lister.js.es6
@@ -1,3 +1,5 @@
+import { INLINE_ONEBOX_LOADING_CSS_CLASS } from "pretty-text/inline-oneboxer";
+
 // to match:
 // abcd
 // abcd[test]
@@ -116,7 +118,7 @@ const DEFAULT_LIST = [
   "a.mention",
   "a.mention-group",
   "a.onebox",
-  "a.inline-onebox-loading",
+  `a.${INLINE_ONEBOX_LOADING_CSS_CLASS}`,
   "a[data-bbcode]",
   "a[name]",
   "a[rel=nofollow]",
diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb
index 6a343be..6a604a3 100644
--- a/lib/cooked_post_processor.rb
+++ b/lib/cooked_post_processor.rb
@@ -33,6 +33,7 @@ class CookedPostProcessor
     DistributedMutex.synchronize("post_process_#{@post.id}") do
       DiscourseEvent.trigger(:before_post_process_cooked, @doc, @post)
       post_process_oneboxes
+      post_process_inline_oneboxes
       post_process_images
       post_process_quotes
       optimize_urls
@@ -575,8 +576,25 @@ class CookedPostProcessor
     @doc.try(:to_html)
   end
 
+  INLINE_ONEBOX_LOADING_CSS_CLASS = "inline-onebox-loading"
+
   private
 
+  def post_process_inline_oneboxes
+    @doc.css(".#{INLINE_ONEBOX_LOADING_CSS_CLASS}").each do |element|
+      inline_onebox = InlineOneboxer.lookup(
+        element.attributes["href"].value,
+        invalidate: !!@opts[:invalidate_oneboxes]
+      )
+
+      if title = inline_onebox&.dig(:tit

GitHub

I don’t think there is anything new here, but we should fix it, we should be allowed to resolve all inline URLs in one go vs N goes from the client.

The particular use case is a post with a lot of links that you edit. If you do that you would flood the server with requests as far as I can tell, no?

1 Like

We actually resolve the inline-oneboxes in batches as per

2 Likes