cnuernber / tmdjs

High Performance Data Processing for ClojureScript
MIT License
100 stars 1 forks source link

Failing to compile with shadow-cljs "release" #5

Closed alex314159 closed 1 year ago

alex314159 commented 1 year ago

Hello Chris,

Using shadow-cljs latest, I can work with tmdjs in a dev environment, but release fails with the following:

PS C:\Users\AAlmosni\IdeaProjects\gui2023> lein prod
[:app] Compiling ...
------ WARNING #1 -  -----------------------------------------------------------
 Resource: ham_fisted/BitmapTrie.js:213:5
 variable i is undeclared
--------------------------------------------------------------------------------
------ WARNING #2 -  -----------------------------------------------------------
 Resource: ham_fisted/BitmapTrie.js:477:8
 variable LFPprops is undeclared
--------------------------------------------------------------------------------
------ WARNING #3 -  -----------------------------------------------------------
 Resource: ham_fisted/BitmapTrie.js:542:22
 variable get is undeclared
--------------------------------------------------------------------------------
------ WARNING #4 -  -----------------------------------------------------------
 Resource: ham_fisted/BitmapTrie.js:611:68
 variable bfn is undeclared
--------------------------------------------------------------------------------
------ WARNING #5 -  -----------------------------------------------------------
 Resource: ham_fisted/BitmapTrie.js:774:53
 variable curEntry is undeclared
--------------------------------------------------------------------------------
------ WARNING #6 -  -----------------------------------------------------------
 Resource: ham_fisted/BitmapTrie.js:864:5
 variable n is undeclared
--------------------------------------------------------------------------------
------ WARNING #7 -  -----------------------------------------------------------
 Resource: ham_fisted/BitmapTrie.js:924:15
 variable put is undeclared
--------------------------------------------------------------------------------
------ WARNING #8 -  -----------------------------------------------------------
 Resource: ham_fisted/BitmapTrie.js:934:20
 variable containsKey is undeclared
--------------------------------------------------------------------------------
------ WARNING #9 -  -----------------------------------------------------------
 Resource: ham_fisted/BitmapTrie.js:935:23
 variable remove is undeclared
--------------------------------------------------------------------------------
------ WARNING #10 -  ----------------------------------------------------------
 Resource: ham_fisted/BitmapTrie.js:945:11
 variable f is undeclared
--------------------------------------------------------------------------------
------ WARNING #11 -  ----------------------------------------------------------
 Resource: ham_fisted/BitmapTrie.js:1059:24
 variable nullEntry is undeclared
--------------------------------------------------------------------------------
------ WARNING #12 -  ----------------------------------------------------------
 Resource: ham_fisted/BitmapTrie.js:1382:51
 variable defaultHashProvider is undeclared
--------------------------------------------------------------------------------
------ WARNING #13 -  ----------------------------------------------------------
 Resource: ham_fisted/ChunkedVec.js:50:10
 variable shallowClone is undeclared
--------------------------------------------------------------------------------
------ WARNING #14 -  ----------------------------------------------------------
 Resource: ham_fisted/ChunkedVec.js:54:20
 variable setOwner is undeclared
--------------------------------------------------------------------------------
------ WARNING #15 -  ----------------------------------------------------------
 Resource: ham_fisted/ChunkedVec.js:146:1
 variable mutAssoc is undeclared
--------------------------------------------------------------------------------
------ WARNING #16 -  ----------------------------------------------------------
 Resource: ham_fisted/ChunkedVec.js:194:12
 variable ChunkedList is undeclared
--------------------------------------------------------------------------------
nil
Closure compilation failed with 2 errors
--- ham_fisted/BitmapTrie.js:518
Class or object literal contains duplicate member "clone". In non-strict code, the last duplicate will overwrite the others.
--- ham_fisted/BitmapTrie.js:542
Class or object literal contains duplicate member "nth". In non-strict code, the last duplicate will overwrite the others.

Error encountered performing task 'run' with profile(s): 'prod'
Suppressed exit

This is my project.clj

:aliases {"dev"  ["with-profile" "dev" "run" "-m" "shadow.cljs.devtools.cli" "watch" "app"]
            "prod" ["with-profile" "prod" "run" "-m" "shadow.cljs.devtools.cli" "release" "app"]}

and shadow-cljs.edn

:builds {:app {:target          :browser
                :output-dir      "resources/public/js/compiled"
                :asset-path      "/js/compiled"
                :modules         {:app {:init-fn gui2023.core/init
                                        ;:preloads [devtools.preload]
                                        }}
                :devtools        {:http-root    "resources/public"
                                  :http-port    8280
                                  :preloads []              ;day8.re-frame-10x.preload
                                  }
                :dev
                {:compiler-options
                 {:closure-defines
                  {                                         ;re-frame.trace.trace-enabled?        true
                   ;day8.re-frame.tracing.trace-enabled? true
                   }}}
                :release
                {:build-options
                 {:ns-aliases
                  {}}}                                      ;day8.re-frame.tracing day8.re-frame.tracing-stubs
                }}
 }

Any idea what needs to be done? thanks,

cnuernber commented 1 year ago

I think I need to fix those issues - they are simple ones to fix. The performance benefits of ham-scripted are 10X+ the base clojurescript hashmap so it is well worth it especially when the datasets grow.

cnuernber commented 1 year ago

I think these are fixed. I cleaned up the errors and warnings and the minimal tests pass in dev compilation mode. If your app doesn't work as expected in release I think it will be because of missing symbols - something didn't get marked as an extern - and I will revisit a bit more thoroughly with the testapp in tmdjs.

2.000-beta-3

alex314159 commented 1 year ago

That worked :)) thank you!

alex314159 commented 1 year ago

I spoke too early I'm afraid. Thanks to your changes the app compiles, however the production js fails with a cryptic JS message (as all the code is compiled) when a (ds/->dataset ...) function is called. Very annoying as dev works perfectly - is there a way I can help narrow down the problem? Using node 19 and shadow 2.18.

Thank you,

cnuernber commented 1 year ago

What is the message?

cnuernber commented 1 year ago

Those really can be tough to debug - I wonder what the best way to debug it is. Perhaps instrumenting some of the code with println will but that will require I think customizing tmdjs.

alex314159 commented 1 year ago

Thanks Chris. The console says:

2app.js:5031 Uncaught TypeError: c is not a function
    at XDa.reduce (app.js:5777:24)
    at h.xa (app.js:5798:34)
    at oh (app.js:4199:62)
    at Tf (app.js:4201:52)
    at h.xa (app.js:4573:29)
    at qd.Da (app.js:4120:427)
    at qd (app.js:4120:191)
    at Object.reduce (app.js:5770:205)
    at qDa (app.js:5741:447)
    at XDa.addAll (app.js:5776:49)

The addAll error is

  qDa(null, (g,k)=>{
                        g.add(k);
                        return g

The reduce error is:

reduce(a, b) {
            a = nDa(a);
            const c = this.kb.$j
              , d = this.length
              , e = this.data
              , f = Math.ceil(d / 32) | 0;
            if (c(b))
                return this.ll.bk(b);
            for (let g = 0; g < f; ++g) {
                const k = e[g]
                  , l = Math.min(32, d - 32 * g) | 0;
                for (let m = 0; m < l; ++m)
                    if (b = a(b, k[m]),
                    c(b))
                        return this.kb.bk(b)
            }
            return b

I will try a minimal example and see if I can learn more.

cnuernber commented 1 year ago

That is the chunked vector reduction code. C in this case is checking if the initial value is reduced. kb is the hash provider.

In any case this looks like a totally legit issue and something I will have to get into; it is in ham-scripted not in tmdjs specifically so perhaps ham-scripted needs release-mode tests.

cnuernber commented 1 year ago

And more specifically it looks like the hash provider's member variables are being 'optimized' differently across modules.

alex314159 commented 1 year ago

Thank you for investigating! I will play around with ham-scripted and see if I find out more!

cnuernber commented 1 year ago

The api declares the real hash provider here. I wonder if that means those declarations are not mangled while the compiler in ChunkedVec is assuming they are and then by extension I wonder if hash provider member variable references should all be quoted strings to stop the compilation.

cnuernber commented 1 year ago

I updated ham-scripted and tmdjs to run unit tests both in debug mode and release mode and fixed all the fallout including exactly your issue.

2.000-beta-5 should work for you but let's keep this issue open until we know for sure.

alex314159 commented 1 year ago

this has worked Chris, legend!! Thank you!

as a small gotcha in java (ds/filter-column dataset col value) works whereas JS seems to want the more exact (ds/filter-column dataset col #(= % value))

cnuernber commented 1 year ago

Ah - I will get to that. The reason is for future proofing. In the event the column has an index, passing in a value or a set of values allows the index to participate in the query. Passing in an opaque function does not.

cnuernber commented 1 year ago

Fixed, scalars and sets are supported and tested.

alex314159 commented 1 year ago

haha nice thanks!

cnuernber commented 1 year ago

Speaking of filtering - since you are working with timeseries data - keep in mind that tech.v3.datatype.argops has a binary-search mechanism that will be far faster than filtering in many cases -

cljs.user> (def small (ds/->dataset {:t (range 50000 51000)}))
#'cljs.user/small
cljs.user> (def big (ds/->dataset {:t (range 51000)}))
#'cljs.user/big
cljs.user> (argops/binary-search (range 51000) 50000)
50000
cljs.user> (time (argops/binary-search (range 51000) 50000))
"Elapsed time: 0.090592 msecs"
"Elapsed time: 0.090592 msecs"
50000
cljs.user> ;;Let's take advantage of the fact the column is sorted.
cljs.user> (defn sorted-take-last
             [ds col val]
             (let [vidx (argops/binary-search (ds col) val)]
               (ds/select-rows ds (range vidx (ds/row-count ds)))))
#'cljs.user/sorted-take-last
cljs.user> (sorted-take-last big :t 50000)
#dataset[unnamed [1000 1]
|    :t |
|------:|
| 50000 |
| 50001 |
| 50002 |
| 50003 |
| 50004 |
|   ... |
| 50994 |
| 50995 |
| 50996 |
| 50997 |
| 50998 |
| 50999 |]
cljs.user> (time (sorted-take-last big :t 50000))
"Elapsed time: 0.553483 msecs"
"Elapsed time: 0.553483 msecs"
#dataset[unnamed [1000 1]
|    :t |
|------:|
| 50000 |
| 50001 |
| 50002 |
| 50003 |
| 50004 |
|   ... |
| 50994 |
| 50995 |
| 50996 |
| 50997 |
| 50998 |
| 50999 |]
cljs.user>
alex314159 commented 1 year ago

super interesting - thanks Chris!