Static viz get background color thread safe (#42548)

src/metabase/pulse/render/color.clj

@@ -16,16 +16,14 @@
 (def ^:private js-file-path "frontend_shared/color_selector.js")
 
 (def ^:private ^{:arglists '([])} js-engine
-  ;; The code that loads the JS engine is behind a delay so that we don't incur that cost on startup. The below
-  ;; assertion till look for the javascript file at startup and fail if it doesn't find it. This is to avoid a big
-  ;; delay in finding out that the system is broken
-  (let [file-url (io/resource js-file-path)]
-    (assert file-url (trs "Can''t find JS color selector at ''{0}''" js-file-path))
-    (let [dlay (delay
-                 (doto (js/context)
-                   (js/load-resource js-file-path)))]
-      (fn []
-        @dlay))))
+  ;; As of 2024/05/13, a single color selector js engine takes 3.5 MiB of memory
+  (js/threadlocal-fifo-memoizer
+   (fn []
+     (let [file-url (io/resource js-file-path)]
+       (assert file-url (trs "Can''t find JS color selector at ''{0}''" js-file-path))
+       (doto (js/context)
+         (js/load-resource  js-file-path))))
+   5))
 
 (def ^:private QueryResults
   "This is a pretty loose schema, more as a safety net as we have a long feedback loop for this being broken as it's

src/metabase/pulse/render/js_engine.clj

@@ -7,6 +7,7 @@
 
   Javadocs: https://www.graalvm.org/truffle/javadoc/overview-summary.html"
   (:require
+   [clojure.core.memoize :as memoize]
    [clojure.java.io :as io]
    [metabase.util.i18n :refer [trs]])
   (:import
@@ -14,6 +15,14 @@
 
 (set! *warn-on-reflection* true)
 
+(defn threadlocal-fifo-memoizer
+  "Returns a memoizer that is unique to each thread."
+  [thunk threshold]
+  (memoize/fifo
+   (with-meta thunk {::memoize/args-fn (fn [_]
+                                        [(.getId (Thread/currentThread))])})
+   :fifo/threshold threshold))
+
 (defn context
   "Create a new org.graalvm.polyglot.Context suitable to evaluate javascript"
   ^Context []

test/metabase/db/query_test.clj

@@ -84,16 +84,6 @@
         (verify-same-query q)))))
 
 
-(defn- repeat-concurrently [n f]
-  ;; Use a latch to ensure that the functions start as close to simultaneously as possible.
-  (let [latch   (CountDownLatch. n)
-        futures (atom [])]
-    (dotimes [_ n]
-      (swap! futures conj (future (.countDown latch)
-                                  (.await latch)
-                                  (f))))
-    (into #{} (map deref) @futures)))
-
 (deftest select-or-insert!-test
   ;; We test both a case where the database protects against duplicates, and where it does not.
   ;; Using Setting is perfect because it has only two required fields - (the primary) key & value (with no constraint).
@@ -121,7 +111,7 @@
                                                            (.countDown latch)
                                                            (.await latch)
                                                            {other-col (str (random-uuid))})))
-                  results (repeat-concurrently threads thunk)
+                  results (set (mt/repeat-concurrently threads thunk))
                   n       (count results)
                   latest  (t2/select-one Setting search-col search-value)]
 
@@ -181,7 +171,7 @@
                                                                   (.countDown latch)
                                                                   (.await latch)
                                                                   {other-col <>}))))
-                    values-set (repeat-concurrently threads thunk)
+                    values-set (set (mt/repeat-concurrently threads thunk))
                     latest     (get (t2/select-one Setting search-col search-value) other-col)]
 
                 (testing "each update tried to set a different value"
@@ -199,7 +189,7 @@
                 (testing "After the database is created, it does not create further duplicates"
                   (let [count (t2/count Setting search-col search-value)]
                     (is (pos? count))
-                    (is (empty? (set/intersection values-set (repeat-concurrently threads thunk))))
+                    (is (empty? (set/intersection values-set (set (mt/repeat-concurrently threads thunk)))))
                     (is (= count (t2/count Setting search-col search-value))))))
 
               ;; Since we couldn't use with-temp, we need to clean up manually.

test/metabase/pulse/render/body_test.clj

@@ -757,12 +757,6 @@
   [render-type card data]
   (body/render render-type :attachment (pulse/defaulted-timezone card) card nil data))
 
-(defn execute-n-times-in-parallel
-  "Execute f n times in parallel and derefs all the results."
-  [f n]
-  (mapv deref (for [_ (range n)]
-               (future (f)))))
-
 (deftest render-cards-are-thread-safe-test-for-js-visualization
   (mt/with-temp [:model/Card card {:dataset_query          (mt/mbql-query orders
                                                                           {:aggregation [[:count]]
@@ -772,10 +766,10 @@
                                    :visualization_settings {:graph.dimensions ["CREATED_AT"]
                                                             :graph.metrics    ["count"]}}]
     (let [data (:data (qp/process-query (:dataset_query card)))]
-      (is (every? some? (execute-n-times-in-parallel #(render-card :javascript_visualization card data) 3))))))
+      (is (every? some? (mt/repeat-concurrently 3 #(render-card :javascript_visualization card data)))))))
 
 (deftest render-cards-are-thread-safe-test-for-table
   (mt/with-temp [:model/Card card {:dataset_query (mt/mbql-query venues {:limit 1})
                                    :display       :table}]
     (let [data (:data (qp/process-query (:dataset_query card)))]
-      (is (every? some? (execute-n-times-in-parallel #(render-card :table card data) 3))))))
+      (is (every? some? (mt/repeat-concurrently 3 #(render-card :table card data)))))))

test/metabase/pulse/render/color_test.clj

@@ -2,7 +2,8 @@
   (:require
    [clojure.test :refer :all]
    [metabase.pulse.render.color :as color]
-   [metabase.pulse.render.js-engine :as js]))
+   [metabase.pulse.render.js-engine :as js]
+   [metabase.test :as mt]))
 
 (def ^:private red "#ff0000")
 (def ^:private green "#00ff00")
@@ -50,3 +51,18 @@
         (is (= [red green red green]
                (for [row-index (range 0 4)]
                  (color/get-background-color color-selector "any value" "any column" row-index))))))))
+
+(deftest render-color-is-thread-safe-test
+  (is (every? some?
+              (mt/repeat-concurrently
+               3
+               (fn []
+                 (color/get-background-color (color/make-color-selector {:cols [{:name "test"}]
+                                                                         :rows [[5] [5]]}
+                                                                        {:table.column_formatting[{:columns ["test"],
+                                                                                                   :type :single,
+                                                                                                   :operator "=",
+                                                                                                   :value 5,
+                                                                                                   :color "#ff0000",
+                                                                                                   :highlight_row true}]})
+                                             "any value" "test" 1))))))

test/metabase/pulse/render/js_engine_test.clj

 (ns metabase.pulse.render.js-engine-test
   (:require
    [clojure.test :refer :all]
-   [metabase.pulse.render.body-test :as body-test]
-   [metabase.pulse.render.js-engine :as js]))
+   [metabase.pulse.render.js-engine :as js]
+   [metabase.test :as mt]))
 
 (set! *warn-on-reflection* true)
 
@@ -23,6 +23,5 @@
     (let [context (js/context)]
       (js/load-js-string context "function plus (x, y) { return x + y }" "plus test")
       (is (= (repeat 10 2)
-             (body-test/execute-n-times-in-parallel
-              #(.asLong (js/execute-fn-name context "plus" 1 1))
-              10))))))
+             (mt/repeat-concurrently 10
+              #(.asLong (js/execute-fn-name context "plus" 1 1))))))))

test/metabase/test.clj

@@ -243,6 +243,7 @@
   secret-value-equals?
   select-keys-sequentially
   throw-if-called!
+  repeat-concurrently
   with-all-users-permission
   with-column-remappings
   with-discarded-collections-perms-changes

test/metabase/test/util.clj

@@ -55,7 +55,7 @@
    (java.io File FileInputStream)
    (java.net ServerSocket)
    (java.util Locale)
-   (java.util.concurrent TimeoutException)
+   (java.util.concurrent CountDownLatch TimeoutException)
    (org.quartz CronTrigger JobDetail JobKey Scheduler Trigger)
    (org.quartz.impl StdSchedulerFactory)))
 
@@ -1409,3 +1409,15 @@
                   {:order-by [[:id :desc]]
                    :where [:and (when topic [:= :topic (name topic)])
                                 (when model-id [:= :model_id model-id])]})))
+
+(defn repeat-concurrently
+  "Run `f` `n` times concurrently. Returns a vector of the results of each invocation of `f`."
+  [n f]
+  ;; Use a latch to ensure that the functions start as close to simultaneously as possible.
+  (let [latch   (CountDownLatch. n)
+        futures (atom [])]
+    (dotimes [_ n]
+      (swap! futures conj (future (.countDown latch)
+                                  (.await latch)
+                                  (f))))
+    (mapv deref @futures)))