Fork me on GitHub
#cljs-dev
<
2015-11-22
>
thheller08:11:28

@dnolen I built a thing, not sure if you'd be interested to have it in cljs.core. basically all of :optimize-constants as a Closure compiler pass instead of what is currently done in the analyzer/compiler. did it as an experiment but it turns out it has some advantages over the current implementation in cljs.

thheller08:11:36

it is java though, so no hope for self-host

dnolen12:11:11

@thheller: looks interesting! what benefits are you seeing?

thheller13:11:44

@dnolen less issues with incremental compilation mostly since it always optimizes everything you don't need to worry about having some files compiled with :optimize-constants and others without

thheller13:11:05

also moves constants to the appropriate module instead of having everything in base

thheller13:11:38

I would have thought that cross module motion would take care of that but somehow it doesn't currently in cljs

thheller13:11:25

:optimize-constants currently has to do a ton of book keeping and checking in caches to make it all work

dnolen13:11:31

yeah I’m not really all that concerned about the first thing - :optimize-constants is rarely explicitly used.

dnolen13:11:50

also less concerned about current implementation

dnolen13:11:08

however the cross module motion bit is an interesting data point.

dnolen13:11:29

and interesting counterpoint would be that optimizing constant data structures - vectors, sets, maps

dnolen13:11:04

is still likely to happen in the future, perhaps a little more annoying to identify after the fact

thheller13:11:32

well technically detecting whether something is constant or not can be done based on the AST as well

thheller13:11:49

so you could do that for vectors, sets, maps as well

thheller13:11:27

but I only did it because I wanted to see how hard it would be

thheller13:11:30

turns out not at all

thheller13:11:07

makes my life easier in shadow-build and my compiles faster

thheller13:11:00

the code motion not working is probably related to all constants being in cljs.core.cst...

thheller13:11:13

not sure why thought since closure already turns those into global vars

dnolen13:11:52

yes just saying would like to see the work involved to see a comparison

dnolen13:11:20

doing keywords and symbols are pretty simple and it’s good to see that’s the case even directly in Closure

thheller13:11:49

3 modules a, b (depends on a), c (depends on a)

thheller14:11:00

the version compiled by cljs has all constants in cljs-base

thheller14:11:17

the version compiled by shadow has them in a,b,c

thheller14:11:23

compile via

thheller14:11:06

don't know why closure doesn't move them given they are globals vars after optimization

dnolen14:11:17

yeah seems weird to me and I could have sworn I have builds where they are moved

dnolen14:11:47

will have to look into it later

thheller14:11:04

another issue I wanted to bring up

thheller14:11:22

not related to the constants but the repo is well suited to demonstrate the issue

thheller14:11:00

there is now a cljs-constants.unused file in the source path

thheller14:11:29

it will end up in cljs-base.js, although no other file requires it anywhere

thheller14:11:16

I think that is broken behavior, but unfortunately some people use it unknowingly

thheller14:11:20

I do not think it should be included in the output at all if nobody requires it

thheller14:11:03

sometimes the issue is not as apparent due to dead code elimination

dnolen14:11:34

sure this just seems like a very minor issue

dnolen14:11:40

feel free to file a minor JIRA ticket

thheller14:11:43

it really isn't

thheller14:11:00

becomes more apparent when you have multiple builds

dnolen14:11:02

that’s the only ticket I’m interested in seeing open on this

dnolen14:11:23

not unless you can show it breaks builds or introduces runtime bugs

thheller14:11:54

IMHO my example already shows both

thheller14:11:06

but we might define those differently

thheller14:11:28

to me a build is broken if something executes that I didn't ask it to

dnolen14:11:34

runtime bugs means the program will throw a visible error

dnolen14:11:46

the build is broken if the source is actually corrupted

dnolen14:11:54

if neither of these apply it is not a major issue

thheller14:11:15

ah ok sorry

thheller14:11:21

i'm fine with minor priority

dnolen14:11:50

sure I understand the problem

dnolen14:11:57

this is something your patch fixes and that is nice

dnolen14:11:26

so another thing add to list of tradeoffs in favor of solving this at the Closure level

thheller14:11:34

hmm but it will break a lot of builds if introduced

thheller14:11:39

that has me worried

dnolen14:11:03

how so? (the breaking part)

thheller14:11:53

mostly in projects that use multi-methods specified in namespaces

thheller14:11:56

but never required

thheller14:11:00

don't remember which project I saw recently that did that

dnolen14:11:03

ah actually yeah it’s a pattern in many applications to have unclear dependencies

dnolen14:11:15

because of the source tree compiling behavior

thheller14:11:43

:main is required to establish what is needed

thheller14:11:32

just was confused that the :modules support you wrote didn't use the entries to elimnate unused files

thheller14:11:35

probably just an oversight

dnolen14:11:41

right but it just assumes inputs is correct

dnolen14:11:52

that the filtering has already been applied