Fork me on GitHub
#cljs-dev
<
2021-05-12
>
dnolen13:05:56

should hopefully fix the long outstanding core.async bugs

lightsaber 2
nice 2
dnolen13:05:15

one interesting result is this is fully generic and does not rely on form tagging

dnolen13:05:42

it will just try to optimize any trivial let/test/if combo

dnolen13:05:08

so actually it optimizes far more than the macro since, even if the first part of a thing can't be optimized, it can optimize the rest

dnolen13:05:28

i.e. (and some.ns/x true false)

dnolen13:05:46

this and fixing handling of #js literals would fix up the two big CLJS core.async pitfalls

dnolen13:05:37

I had stalled a bit on this previously because I thought type inference was needed

dnolen13:05:45

but now it's clear that this pass just runs after type inference

dnolen14:05:08

@dpsutton nice - still working through the test cases

dpsutton14:05:23

i'm interested to see what you do for that

dnolen14:05:44

just a bunch of annoying variations but we should have explicit tests

dnolen14:05:52

I want to turn this on and know it's more or less going to work

dpsutton14:05:39

here are the two repros from those issues if you want them handy

dnolen14:05:50

@dpsutton yeah I'm not so worried about the core.async tests

dnolen14:05:01

just removing the and/or macro stuff makes the problem go away of course

dnolen14:05:18

the tests I'm writing now are about checking that all desirable scenarios do optimize

dnolen14:05:26

the and/or optimizations are on the critical path

dnolen14:05:31

it's probably not obvious why, but it's one of the most important optimizations we do

dnolen14:05:25

a quick summary is that and/or produce let

dnolen14:05:39

but let can go anywhere in the emitted code, so it's wrapped in an IIFE

dnolen14:05:44

but that is a performance destroyer

dnolen14:05:13

compound boolean expressions are in the persistent data structure code

dnolen14:05:25

this stuff can never include an IIFE

dnolen14:05:28

nor a truth test

dpsutton15:05:56

oh i knew that optimization was important but didn't think about it being used in the core data structures

dnolen15:05:04

probably the single most important automatic optimization for that code

dnolen15:05:49

otherwise would have to write really strange ClojureScript

dpsutton15:05:32

yeah makes sense. just hadn't considered that.

dnolen19:05:37

eye balling the persistent data structure code looks good

dnolen19:05:23

no detectable regression in compiler performance at least from compiling all of the runtime tests

dnolen20:05:44

would be nice to give master a try on your projects if you have a chance

kommen20:05:47

sure, anything in particular we should pay attention to?

kommen20:05:39

also no detectable compiler performance change for our production build

kommen21:05:01

and nothing apparent wrong at runtime either after just a bit of testing

raspasov21:05:37

Just bumped up to https://github.com/clojure/clojurescript/commit/927f221f8fc26a49db7d0dcfd1d70008a986fd8f . I get this error when compiling during dev: [Wed May 12 2021 13:54:01.150]  ERROR   [TypeError: logger_SINGLEQUOTE_.setLevel is not a function. (In ‘logger_SINGLEQUOTE_.setLevel(lvl)’, ‘logger_SINGLEQUOTE_.setLevel’ is undefined)] … but I have a feeling that’t not part of ClojureScript per-se. I have a hard time isolating where it’s coming from at the moment. The stacktrace does not look very helpful.

raspasov21:05:14

Going to try compiling & running under :advanced

kommen21:05:21

@raspasov from which version did you bump? this sounds more like one of the breaking changes from the .844 release: https://clojurescript.org/news/2021-04-06-release#_noteworthy_breaking_changes

raspasov21:05:47

From the latest (I’m aware of those)

raspasov21:05:57

From 1.10.844

kommen21:05:25

ok, then sorry for my noise

raspasov21:05:33

No problem.

raspasov21:05:33

:advanced seems fine. Perhaps something in figwheel-main during :dev. Gotta investigate more.

raspasov22:05:59

Yes, it’s from figwheel-main

dnolen22:05:20

@raspasov were you on 844 before

dnolen22:05:52

Oh sorry see the backlog - seems odd though there were some other changes

dnolen22:05:51

For the and/or change no news is the best news

dnolen22:05:18

I guess and/or change could cause runtime improvements but probably only in lower level code

raspasov22:05:19

I’m guessing (could be wrong) because there’s an if statement that depends on the clojurescript-version it might not be selecting the proper logger since it’s a :sha version? https://github.com/bhauman/figwheel-repl/blob/master/src/figwheel/repl/logging.cljs#L19

raspasov22:05:46

But this is a figwheel issue; As far as I can tell, the ClojureScript changes are fine.