Fork me on GitHub
#cljs-dev
<
2017-06-17
>
thheller09:06:45

@dnolen forgive my ignorance but what exactly does inlining ^:const solve?

thheller09:06:48

(def ^:const a-const {:hello ["something" {:really #{:nested 1}}]})

(js/console.log "identical?" (identical? a-const a-const));; => false

thheller09:06:54

maybe I misunderstand the goal of :const but I would expect a constant to always be identical with itself?

Oliver George09:06:48

27 commits to master on Friday.

rauh09:06:39

This seems to break the :const stuff, I get a "Failed compiling" without a more specific error message:

(def ^:const content-type-header
  "A JS object with Content-Type key and the transit header."
  #js{"Content-Type" "application/transit+json; charset=utf-8"})

rauh09:06:49

Using js-obj instead of the reader macro works fine.

rauh10:06:15

Also the :const inlining doesn't seem to work with (def ^:const (some-macro))

dnolen10:06:26

@rauh assume for now it only works with regular EDN values

dnolen10:06:38

And no I don't expect that macro thing to work, but double check Clojure behavior

dnolen10:06:43

@thheller it's just for substitutions and I don't think the identical? property is important

thheller10:06:16

but the :const label is very misleading imho

dnolen10:06:30

Noted but not concerned

rauh10:06:40

Sorry, I made a mistake above, It should be: (def ^:const some-var (some-ns/some-macro))

rauh10:06:11

The inlining will make this "some-ns" not defined error

thheller10:06:58

(def ^:const foo (do (prn :foo) 1)) this also doesn’t look like a const since the prn is triggered every time it is inlined

rauh10:06:26

I think clojure just precomputes :const during compilation stage, kinda like a macro except it's not called. Eg this code: https://gist.github.com/rauhs/fbf9e184d41447daf7d90101a463766a

dnolen10:06:55

@thheller no reason for that to work

dnolen10:06:17

Again double check with Clojure behavior and we'll see what we care about

thheller10:06:17

user=> (def ^:const foo (do (prn :foo) 1))
:foo
#'user/foo
user=> foo
1

thheller10:06:19

works in clojure

dnolen10:06:24

Value must be static expression

dnolen10:06:53

@thheller you should probably check some more things - like AOT in Clojure

dnolen10:06:19

If you're not checking that then you don't know

dnolen10:06:49

^:const breaks redef in Clojure intentionally

dnolen10:06:39

@rauh we could try to evaluate the constant initializer but I don't really care about that use case.

dnolen10:06:57

This basically goog-define not limited to EDN

dnolen10:06:18

Not limited to true string number I mean

rauh10:06:19

Yeah it's not important, GCC will do the precomputing for us. And it's been inlining all the @const.

rauh10:06:59

Just think, that the inlining should be maybe limited, to strings/numbers/regexes (etc)?

thheller10:06:45

not sure what redefs have to do with it but if you say it is working as intended I take you word for it

thheller10:06:54

don’t understand the intent but that is fine I don’t have to use it

dnolen10:06:19

Just that testing at the REPL isn't telling you anything

thheller10:06:39

I just tested it AOT compiled … same thing

rauh10:06:53

This change will need a big warning message upon release or we'll have the channels flooded with errors. I needed to remove quite a bit of :const for my project to compile

thheller10:06:00

yes it prints more when compiling but not at runtime

dnolen10:06:20

@rauh you were doing non-literally?

rauh10:06:55

Yeah, I was def'ing const with macros, which return a string literal.

rauh10:06:07

And then the #js{....} didn't seem to work, that's it though

dnolen10:06:59

So then I think we should avoid the behavior if the expression is not constant EDN

rauh10:06:09

I agree with that.

dnolen10:06:13

Value has to be immutable

rauh10:06:33

Should it macroexpand beforehand?

dnolen10:06:37

So will probably warn about non EDN literal s

dnolen10:06:17

We could macroexpand as enhancement yes

dnolen10:06:52

I don't really think Clojure behavior here is replicatable because of runtime compile time divide

dnolen10:06:07

So not super interested in eval

dnolen10:06:48

Related I wonder if @nosideeffect annotation in top level literals would fix our DCE issues?

rauh10:06:01

Not allowed, only in externs.

thheller10:06:04

@nosideeffect is for externs only

dnolen10:06:19

So there's no way to hint that inside?

rauh10:06:22

I wish we had that... And I wish we had noinline

rauh10:06:51

No, there is no way to hint @nosideeffect, the official response is that "GCC should be able to detect this", so the annotation isn't allowed

dnolen10:06:21

Hrm obviously it cannot detect it

dnolen10:06:21

Hasn't been able to in 6 years and counting

rauh10:06:35

Yeah it's terrible with cljs code at detecting this. Even a (concat [] ["a"]) stays in there.

dnolen10:06:27

hrm I wonder if marking all of our constructors @final makes any difference to DCE?

dnolen10:06:57

but I suspect not

dnolen10:06:28

or maybe? worth playing around

dnolen10:06:00

hrm also @struct for deftype/record

dnolen12:06:31

hrm I think we might be wrong

mfikes12:06:43

Just catching up on the above after having filed https://dev.clojure.org/jira/browse/CLJS-2097. In this case I had ported some working Clojure code which was using ^:const (https://github.com/AvisoNovate/pretty/blob/master/src/io/aviso/ansi.clj#L8-L20) to ClojureScript. Perhaps we are considering this might be invalid code in ClojureScript?

dnolen12:06:46

I can’t actually recreate DCE problems with top level Clojure values anymore

dnolen12:06:07

@mfikes yeah I think after @rauh’s report only attempting to inline actual static values is probably the way to go

dnolen12:06:11

otherwise too many problems

mfikes12:06:50

Ahh, so the scope of the optimization might be reduced from EDN to literals like 1 and "a"?

mfikes12:06:26

In other words you couldn't do (def ^:const a [1])

mfikes12:06:55

Or, are we saying that the code is invalid? For example (def ^:const (str a)) should not have the ^:const on it.

dnolen12:06:49

right to clarify

dnolen12:06:02

I’m just saying the inlining behavior should only apply to EDN literals

mfikes12:06:38

I wonder if the compiler can detect that automatically and de-opt when not.

dnolen12:06:52

yes that’s what I’m going to do right now 🙂

dnolen12:06:06

we should definitely adjust our expectations about what Closure can do with top level ClojureScript data structures now - like I said I could not recreate the old problems here anyway with trivial examples

thheller12:06:44

which “old problems” are you referring too?

dnolen12:06:08

top level values wouldn’t get DCE’d at all

dnolen12:06:11

I cannot recreate this

rauh12:06:50

I agree, it used to be much worse. Now it seems the DCE does remove top level data structures.

mfikes12:06:54

Even if it turns out to not be feasible to detect clean EDN, something along these lines might still be useful

-          (when const?
+          (when (and const? ('#{boolean number string} (:tag init-expr)))

rauh12:06:05

As long as they're not created by something like asIfAssoc

dnolen12:06:02

@mfikes detecting real static EDN is pretty simple 🙂

mfikes12:06:12

Ahh. Nice. 🙂

thheller12:06:56

(def x (atom 1)) still never gets removed because of the declare issue

thheller12:06:47

more specifically it doesn’t get removed because of the .call usually ends up as something like Nc||Mc.call(null,1);

rauh12:06:56

Yeah I've initially considered adding that to that deref ticket from yesterday. But kept the scope small.

rauh12:06:38

Same for reset!

thheller12:06:55

it doesn’t get removed because it goes through the cljs.core.atom dispatch fn and not the cljs.core.atom.cljs$core$IFn$_invoke$arity$1

rauh12:06:17

@thheller Btw, the patch in 1992 has the filename 1994, and it doesn't apply to master right now

dnolen12:06:26

is it the dispatch fn or the .call pattern that creates the DCE issue though?

thheller12:06:54

calling the dispatch fn via .call is

thheller12:06:19

hmm actually it might just be the dispatch fn

thheller12:06:37

don’t think ever those ever get removed either

dnolen12:06:39

right it would be good to know what to fix

thheller12:06:29

@rauh oops, I’ll update the patch

dnolen12:06:57

@rauh @mfikes try master when you get a chance, hopefully it means you don’t have to change anything

mfikes12:06:14

Thanks @dnolen ! Trying now. 🙂

thheller12:06:39

hehe about that declare bug .. just noticed this

mfikes12:06:41

On the side, I'm seeing if I can break case in any way with this new feature. It is nice: The test values put into the switch are inlined 🙂

thheller12:06:41

cljs.user=> (source atom)
(declare create-inode-seq create-array-node-seq reset! create-node atom deref)

rauh12:06:25

So master doesn't produce valid JS for me right now. Investigating.

mfikes13:06:24

@thheller I evidently failed to file an upstream bug when I noticed that 😞 https://github.com/mfikes/planck/issues/188

rauh13:06:28

This:

(def ^:const path-mask 63)
(defn path-get
  "Gets inlined by GCC"
  [path level]
  (bit-and path-mask
           (unsigned-bit-shift-right path level)))
PRoduces:
eav.btset.path_get = (function eav$btset$path_get(path,level){
return ((63);
 & (path >>> level));
});

mfikes13:06:00

I'll file a JIRA now @thheller

thheller13:06:29

@mfikes it would be fixed by https://dev.clojure.org/jira/browse/CLJS-1992 don’t think it needs a new issue

thheller13:06:28

@dnolen quick test shows that it must be the .call

thheller13:06:33

(ns test.foo)

(defn atom-sig
  ([x] x)
  ([x & {:keys [meta validator]}] [x meta validator]))
gets removed completely

dnolen13:06:38

@thheller 1992 looks fine, but yeah lets fix the patch

thheller13:06:43

which is the same signature atom has

thheller13:06:00

fixing patch now

mfikes13:06:05

script/test-self-parity is failing right now, will see which commit regressed it: https://gist.github.com/mfikes/f6e7caa45a026608621a459cb703ff75

dnolen13:06:08

@mfikes yeah you might want to wait need to fix this

mfikes13:06:35

Planck is also failing to compile in an odd way that I haven't sorted with the last commit

dnolen13:06:28

regular tests & self host parity works for me now

dnolen13:06:10

@rauh I couldn’t recreate your issue with master

rauh13:06:13

@dnolen All working, even the #js const and the macro const.s

dnolen13:06:30

yes, this is a very conservative change now 🙂

mfikes13:06:55

@dnolen Things are clean for me now. The case I have this blog post runs an order of magnitude faster now http://blog.fikesfarm.com/posts/2015-06-15-clojurescript-case-constants.html (putting together a gist showing that)!

dnolen13:06:23

@mfikes I thought your change inlined ^:const why is this so much faster now?

mfikes13:06:07

case used to compile down to use the JavaScript variables for ^:const expressions as tests. Now they are inlined with the literal values

mfikes13:06:33

Instead of

case foo.bar.m:
    return 2;
    break; 

mfikes13:06:56

yeah, you get it 🙂

dnolen13:06:00

so your change was just to support putting them into case

dnolen13:06:09

but now they just get inlined

dnolen13:06:14

that is actually pretty cool

dnolen13:06:23

for writing higher performance code

dnolen13:06:38

so you don’t have to use magic constants, but assign them names

mfikes13:06:20

I didn't make any changes to case, historically, that I recall. Perhaps someone made it use JavaScript switch constructs under some conditions... but yeah, the main speedup here is due to the inlining of constants.

dnolen13:06:12

@mfikes oh hrm, I thought that’s what you said yesterday that you did something with ^:const

dnolen13:06:17

maybe I misunderstood

dnolen13:06:15

@mfikes I did work a bit on the switch thing along with some other people

dnolen13:06:12

it’s true in theory that Google Closure should do @const replacement, but it’s nice to just get this feature without going :advanced

dnolen13:06:42

@thheller yes testing now

thheller13:06:26

I’m not sure how to write a proper test for this

rauh13:06:50

@thheller Just tried the patch. It works great. All the derefs got inlined to -deref 🙂

dnolen13:06:52

well just smoke testing by running all the suites and REPL 🙂

dnolen13:06:14

I agree it’s annoying to write a proper test for

rauh13:06:14

You could use the fact that it won't generate proper calls in clojure/cljs/compiler_tests.clj

thheller13:06:18

hmm I have an idea, can just check the analyzer env

dnolen13:06:46

@rauh trying CLJS-2046, looks OK, thanks for doing that

rauh13:06:54

No prolem. The tests took me longer than the patch 😄

thheller14:06:26

(deftest test-cljs-1992 ;; declare after def should have no effect
  (let [test-cenv (e/default-compiler-env)]
    (e/with-compiler-env test-cenv
      (a/analyze-form-seq
        '[(ns test.cljs-1992)
          (defn test-fn [a b c] :foo)
          (declare test-fn)]
        ))

    (let [def (get-in @test-cenv [::a/namespaces 'test.cljs-1992 :defs 'test-fn])]
      (is (:fn-var def)))))

thheller14:06:33

that seems to do it

thheller14:06:43

second patch or change first one?

dnolen14:06:09

@thheller let’s do it one patch

thheller14:06:29

ok, adding now

dnolen14:06:36

@rauh nice the patch seems to collapse the difference between (get x :foo) and (:foo x)

dnolen14:06:47

2046 applied to master

rauh14:06:26

@dnolen Awesome! Thanks. So about 2041... 😄

dnolen14:06:39

@rauh while I’m testing this, I think the name of the config is misleading :fn-invoke-call

dnolen14:06:54

since it’s what we currently do and in the patch this is false by default

dnolen14:06:00

but if it’s true we elide .call

dnolen14:06:20

let’s change this to :fn-invoke-direct and change the dyn var as well

dnolen14:06:28

so we start false and we enable it

rauh14:06:38

Yeah IIRC, my inital suggestion was something like assume-fast-invoke or something like it.

rauh14:06:48

I'll make the changes

dnolen14:06:59

@mfikes I’m somewhat curious if 2046 affects compile time significantly, if you can run your compile time thing that would be cool 🙂

dnolen14:06:05

we really need some CI here 🙂

mfikes14:06:00

On the CI front, I got the Travis setup that Rohit was using, so that's a start 🙂

rauh14:06:53

@dnolen Updated patch in 2041 (rename).

dnolen14:06:01

@rauh with your patch and :fn-invoke-call true tests & self host tests pass

mfikes14:06:19

I looked through the code trying to find anything that could be turned into a ^:const and this was the only thing, but to truly see if it does anything, it might only improve self-hosted compilation speeds (which I haven't checked): https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/compiler.cljc#L367

rauh14:06:32

I didn't see significant increases in compile time with 2046, I was wondering that too. It's pretty rare to do a higher order invoke like that.

rauh14:06:13

The only thing is that people should prefer to do (get @x y) instead of (@x y) because of the potential IIFE, but they're also very fast so end result should still be faster than going thru the dispatch

dnolen14:06:13

@rauh yeah I didn’t either

dnolen14:06:17

2041 applied

rauh14:06:50

Awesome! Thanks a ton!

mfikes14:06:38

@dnolen CLJS-2046 doesn't seem to affect the time needed to compile Planck. (Around 15 seconds both with and without the change.)

dnolen14:06:47

k cool, thanks for the confirm

rauh14:06:22

@dnolen With the change from Thomas, this becomes obsolete: https://dev.clojure.org/jira/browse/CLJS-2096

rauh14:06:34

Close or still apply for the cleanup?

dnolen14:06:16

leave it open, can apply it as a clean up thing later

dnolen14:06:31

I’ve marked the issue trivial

mfikes19:06:42

Got some CI sorted out with an initial patch in https://dev.clojure.org/jira/browse/CLJS-2098. Here is what it looks like running on a fork, if you're curious: https://travis-ci.org/mfikes/clojurescript Credit to Rohit. And António has ideas surrounding Docker that could make it better.