From 35a487d367112e199d7e25a68923154321188126 Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Mon, 13 May 2024 22:17:44 +0700
Subject: [PATCH] Static viz get background color thread safe (#42548)
---
src/metabase/pulse/render/color.clj | 18 ++++++++----------
src/metabase/pulse/render/js_engine.clj | 9 +++++++++
test/metabase/db/query_test.clj | 16 +++-------------
test/metabase/pulse/render/body_test.clj | 10 ++--------
test/metabase/pulse/render/color_test.clj | 18 +++++++++++++++++-
test/metabase/pulse/render/js_engine_test.clj | 9 ++++-----
test/metabase/test.clj | 1 +
test/metabase/test/util.clj | 14 +++++++++++++-
8 files changed, 57 insertions(+), 38 deletions(-)
diff --git a/src/metabase/pulse/render/color.clj b/src/metabase/pulse/render/color.clj
index 81456d5a337..8c05ad76c02 100644
--- a/src/metabase/pulse/render/color.clj
+++ b/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
diff --git a/src/metabase/pulse/render/js_engine.clj b/src/metabase/pulse/render/js_engine.clj
index 916a0cce3aa..6a331924599 100644
--- a/src/metabase/pulse/render/js_engine.clj
+++ b/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 []
diff --git a/test/metabase/db/query_test.clj b/test/metabase/db/query_test.clj
index 385b1403952..3c8f5d01956 100644
--- a/test/metabase/db/query_test.clj
+++ b/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.
diff --git a/test/metabase/pulse/render/body_test.clj b/test/metabase/pulse/render/body_test.clj
index 47aa3ca8d41..55d758a7233 100644
--- a/test/metabase/pulse/render/body_test.clj
+++ b/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)))))))
diff --git a/test/metabase/pulse/render/color_test.clj b/test/metabase/pulse/render/color_test.clj
index 433252bfb45..d5a9d617ef2 100644
--- a/test/metabase/pulse/render/color_test.clj
+++ b/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))))))
diff --git a/test/metabase/pulse/render/js_engine_test.clj b/test/metabase/pulse/render/js_engine_test.clj
index 9f194ee7536..79eca9b9bed 100644
--- a/test/metabase/pulse/render/js_engine_test.clj
+++ b/test/metabase/pulse/render/js_engine_test.clj
@@ -1,8 +1,8 @@
(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))))))))
diff --git a/test/metabase/test.clj b/test/metabase/test.clj
index 0d621a0777d..90c0387797e 100644
--- a/test/metabase/test.clj
+++ b/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
diff --git a/test/metabase/test/util.clj b/test/metabase/test/util.clj
index ad8152fd56d..11e6500e79a 100644
--- a/test/metabase/test/util.clj
+++ b/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)))
--
GitLab