Fork me on GitHub
#cljs-dev
<
2017-06-30
>
mfikes02:06:50

Patch attached to CLJS-2139—it would re-enable analysis diagnostics for defn (and named fn) bodies.

mfikes02:06:33

Added an analyzer test as well

mfikes13:06:31

No problem. That was a pretty bad regression that went by unnoticed. 😵

mfikes13:06:12

Glad it was just inadvertent suppression of warning diagnostics and that we didn’t need to give up the optimization. 🙂

dnolen13:06:22

well we didn’t have a test for it 🙂

mfikes13:06:41

(I only recently learned about writing analyzer tests. Will do so in future.)

dnolen13:06:32

@mfikes is there a particular reason you want :simple tests also under CI?

mfikes13:06:58

@dnolen No. Assumed that they might be good for coverage. But if you'd like CI to complete more quickly, could decline that one.

dnolen13:06:06

also unless there’s something pressing, going to cut a fix release

dnolen13:06:03

@mfikes I just can’t think of a case where :simple will tell you something more than :advanced - though I suppose :simple will give you more meaningful looking failures

mfikes13:06:31

@dnolen Right. I assumed there might have been a historical "coverage" rationale for script/test-simple but I was wondering the same (whether things ever fail that way but not in :advanced)

mfikes13:06:29

Sounds like those tests may not offer any value in CI, while slowing it down

dnolen13:06:27

@mfikes pretty sure :simple tests only exist to make it easy to understand :advanced failures

dnolen13:06:53

which is useful, but perhaps not important for CI?

mfikes13:06:54

Cool. With that understanding, I'd decline that patch

favila13:06:03

I vaguely remember some failures that got DCEd by advanced

favila13:06:01

let me try to recover the situation. it may not be relevant

favila13:06:51

nevermind I don't remember, and though I know I wrote about it I can't find where

mfikes13:06:34

An interesting perf observation: If I enable :optimize-constants in Planck (which is self-host :none) it works properly, but there is no effect on compilation speed (tested by require of cljs.core.async), but it causes an extra 200 ms (on top of 1200 ms) startup latency. Hypothesis is that for this environment, allocation is cheap, while loading the full constants table is not. (This might also be applicable for React Native apps, where it seems devs are dispensing with :advanced, but where launch latency is important).

dnolen13:06:44

that’s interesting

dnolen13:06:54

cutting 1.9.671

favila13:06:11

@mfikes that seems like a good hypothesis. Lazy parsing may be involved, too: constants table would frustrate lazy parsing efforts

favila13:06:40

those 200ms would be parsing and creating the constants that were not needed at startup

favila13:06:33

I wonder if there's some other way to express the constants table that works better with lazy parsing

mfikes13:06:12

I'm happy we have the knobs for all these use cases :)

dnolen14:06:54

1.9.671 released

mfikes17:06:03

I'm taking a stab at https://dev.clojure.org/jira/browse/CLJS-2142 and instead of attempting to force the issue (allowing set! on ^:const Vars), I'm thinking of taking the approach that those kinds of Vars don't fall into the "instrumentable" set referenced in the docstring.

dnolen17:06:29

@mfikes yeah that’s what I’m thinking, should be filtered out

mfikes17:06:57

Cool @dnolen. Easy enough to do and adding a test covering that case as well.

dnolen17:06:07

great thanks

mfikes17:06:36

Not a regression with related to ^:const Var inlining FWIW... been there for a while.