Fork me on GitHub
#cljs-dev
<
2017-07-02
>
mfikes00:07:55

The aget / aset warning patch found 15 abuses in the std lib and a handful more in the test suite. (Supplied fixes for those in a revised patch in CLJS-2148)

dnolen00:07:16

@mfikes might be nice to have those in separate patch

mfikes00:07:29

OK. Will split them into two.

dnolen00:07:41

this new warning is guaranteed to create chaos 🙂

dnolen00:07:52

probably one of the few warnings we want to put behind a false flag

dnolen00:07:57

and people can enable it for code style

mfikes00:07:14

Yes, and it would be nice to separate the warning itself from all of the (perhaps slightly dangerous) fixes for the warning.

mfikes00:07:40

Ahh, yeah, we can have them default to false.

mfikes00:07:10

As David suggested, there is now a CLJS-2148-v2.patch attached to https://dev.clojure.org/jira/browse/CLJS-2148 that you can try on your codebases to see how many abuses of aget and aset it can find. The warnings are turned off by default, so you'd need to enable them.

mfikes12:07:29

If code uses aget / aset for object access, then it makes it difficult to do anything about https://dev.clojure.org/jira/browse/CLJS-2149

mfikes12:07:44

Not impossible; just difficult to do efficiently while preserving abuse

mfikes12:07:50

Perhaps the code emitted could depend on what type the analyzer infers. That would be a mess, but would shove some of the inefficiency out of runtime to compile time.

dnolen16:07:57

I’ve tested the module-loader-preflight branch quite a bit - all tests (core, lein, self-host, self-parity), all REPLs (browser, nashorn, rhino, node), JS module guides, on Om (w/ Figwheel), on a trivial cljsjs.react project and I tested this new guide of course.

dnolen16:07:38

but more testing would be good as it required touching quite a bit of code so that we could compute the information required by ModuleManager in straightforward way.

dnolen16:07:10

while thinking through the :none where we need to load stuff dynamically - I went ahead and re-considered what @thheller said about assigning compiler inputs to modules - and I copied what Google Closure JSModuleGraph does more or less.

dnolen16:07:13

pretty huge change so I’m less concerned about code review and more just testing 🙂

dnolen16:07:37

also can’t believe I just realized that java -cp cljs.jar clojure.main -m cljs.repl.browser works for starting a local webserver 🙂

mfikes16:07:06

AFK, but wondering if java -jar cljs.jar -m cljs.repl.browser also works :)

dnolen16:07:49

right, we should make this a hair nicer so you can get just the simple webserver and not the REPL

dnolen16:07:36

I’ve been instinctively using python -m SimpleHTTPServer 8000 but this isn’t so nice for tutorials

mfikes17:07:34

The above looks like really cool stuff. I'm wondering if might even be useful for gigantic React Native apps (where all the code is local, but you want to bring up the initial screen with minimal latency. I doubt it would matter for that use case in the end, given how quickly JavaScript can be loaded. Also wondering about self-host low-latency loading (understood if none of this works in self-hosted).

dnolen17:07:11

hrm hard to say - code splits are driven by network latency - so your parse times would probably be have to be worse than that to be impactful?

anmonteiro18:07:53

Anyone seen this before?

dnolen18:07:47

@anmonteiro no but the error is quite strange

dnolen18:07:55

it looks like it tried to find a macro in a .cljs file no?

anmonteiro18:07:19

Looks like it. I wonder if we still have a bug in cljs.js-deps

dnolen18:07:10

@anmonteiro is this with master?

anmonteiro18:07:23

I'm also going to disable :parallel-build from now on since it could be the case that there's a bug in there

anmonteiro18:07:30

This is with 1.9.671

dnolen18:07:53

hrm, I don’t see how that could cause macro lookup in the wrong file type (but maybe)

anmonteiro18:07:07

Trying to get the new Lumo release through Homebrew

anmonteiro18:07:48

It only ever seems to happen on OSX Yosemite though

anmonteiro18:07:07

I don't know what kind of conditions are in place for this to be happening

dnolen18:07:26

well hopefully someone else can repro

dnolen18:07:54

I mean seems common enough situation that someone else might run into it

bronsa18:07:39

I think we saw something similar in one of our builds the other day

bronsa18:07:51

don't have access to the build logs ATM but will check tomorrow

anmonteiro18:07:07

just verified it happens on Windows too (and macOS Sierra). opened https://dev.clojure.org/jira/browse/CLJS-2153

anmonteiro18:07:15

here’s the Windows build where the problem occurs: https://ci.appveyor.com/project/anmonteiro/lumo/build/815

anmonteiro18:07:33

java.lang.Thread.run              Thread.java:  745
...
clojure.core/bound-fn*/fn                 core.clj: 2000
clojure.core/apply                 core.clj:  661
...
clojure.core/with-bindings*                 core.clj: 1970 (repeats 2 times)
clojure.core/apply                 core.clj:  657
...
cljs.closure/parallel-compile-sources/fn              closure.clj:  889
cljs.closure/eval6343/fn/G              closure.clj:  450
cljs.closure/eval6413/fn              closure.clj:  576
cljs.closure/compile-from-jar              closure.clj:  548
cljs.closure/eval6343/fn/G              closure.clj:  450
cljs.closure/eval6407/fn              closure.clj:  566
cljs.closure/compile-file              closure.clj:  497
cljs.compiler/compile-file            compiler.cljc: 1436
cljs.compiler/compile-file/fn            compiler.cljc: 1459
cljs.compiler/compile-file*            compiler.cljc: 1370
cljs.compiler/with-core-cljs            compiler.cljc: 1206
cljs.compiler/compile-file*/fn            compiler.cljc: 1381
cljs.compiler/emit-source            compiler.cljc: 1307
cljs.analyzer/analyze            analyzer.cljc: 3352
cljs.analyzer/analyze*            analyzer.cljc: 3328
clojure.core/reduce                 core.clj: 6703
...
cljs.analyzer/analyze*/fn            analyzer.cljc: 3328
cljs.analyzer/ns-side-effects            analyzer.cljc: 3265
cljs.analyzer/check-use-macros-inferring-missing            analyzer.cljc: 2055
...
clojure.core/update-in                 core.clj: 6054
clojure.core/update-in                 core.clj: 6068
clojure.core/update-in/up                 core.clj: 6067
clojure.core/apply                 core.clj:  659
...
cljs.analyzer/check-use-macros-inferring-missing/fn            analyzer.cljc: 2057
cljs.analyzer/check-use-macros            analyzer.cljc: 2043
cljs.analyzer/error            analyzer.cljc:  655
cljs.analyzer/error            analyzer.cljc:  657
clojure.core/ex-info                 core.clj: 4725
clojure.lang.ExceptionInfo: Invalid :refer, macro cljs.spec.gen.alpha/lazy-prims does not exist in file C:\Users\appveyor\.boot\cache\tmp\projects\lumo\18g\-gi273p\main.out\cljs\spec\gen\alpha.cljs


anmonteiro18:07:39

so ^ here’s the stacktrace

anmonteiro18:07:57

seems related to the analyzer and not js-deps

thheller18:07:35

@dnolen I don’t see a :module-loader config option in your example? I think the loader should be optional since it adds quite a few dependencies itself

anmonteiro18:07:45

I wonder if there could be some sort of concurrency bug in acquiring these locks: https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/analyzer.cljc#L3244-L3262

anmonteiro18:07:59

sounds unlikely, but I don’t have any other ideas ATM

rauh19:07:54

Further working on apply speedup. It's now beating CLJ in a few cases 🙂

dnolen19:07:01

@thheller it is optional your ns has to require it. No need for flag.

thheller19:07:21

ah, good point

mfikes23:07:25

@anmonteiro if the concurrency thing is relatively reproducible, perhaps a git bisect, or a coarse-grained "first compiler version" would let us pinpoint. I was looking at this recent commit, wondering if the ::no-resolve could derail things somehow in concurrent code https://github.com/clojure/clojurescript/commit/01a1427e45c9726244940baeeb37b7511acbd8b1

mfikes23:07:17

(Maybe I can try to repro when I again have access to my 12-core box.)

anmonteiro23:07:17

that doesn’t look very offensive to me

anmonteiro23:07:52

the problem with this issue is that it’s non-deterministic (hence me thinking about parallel build)

anmonteiro23:07:59

I can’t repro it locally for example

anmonteiro23:07:05

so far it has only shown up in CI

mfikes23:07:17

That reminds me, I've turned off :parallel-build for Planck owing to a fairly reproducible Transit-related bug I need to track down. Maybe I can spend some time with Lumo with :parallel-build as well and try to shake out any concurrency bugs.

anmonteiro23:07:52

by all means 🙂