Fork me on GitHub
#cljs-dev
<
2018-01-09
>
vemv08:01:42

hi all, if you don't mind a sneaky question: at work our project has 586 cljs files (not counting tests), 60K LOC total, including white lines these are the compilation sizes with most dependencies removed (as an experiment): :advanced = 3.5MB :simple - 13.2MB :none - 45MB again, this is just with core dependencies (React, Om, Sablono, core.async) and removing ~15 dependencies (and therefore some other transitive dependencies).

vemv08:01:34

my question is: is this surprising/expected? could it be considered a compiler issue?

vemv08:01:24

or is not reasonable to expect lower sizes these days

rauh08:01:41

Not sure if this is the right channel. But to answer your question: You need to measure what namespaces are taking up the space. FWIW, i have a project about half the size with ~700KB output size. (W/o react though).

vemv09:01:12

we don't get a source map to begin with 😞 I think it stops working past a size thanks for the measuring suggestion! I'll look at the directory tree with optimizations :none and note any outliers finally, I know what #cljs-dev is for, was hesitant about asking this but sincerely I find it a question few should be able to answer. as emphasized, I genuinely don't know if this can be considered a compiler issue

juhoteperi08:01:20

@thheller do you think one should use :default thing with every CJS module also?

juhoteperi08:01:48

Maybe that'd be okay as the whole module support is still experimental

thheller08:01:46

@juhoteperi :default is a replacement for the directly invokable :as. CLJS doesn’t allow directly invokeable namespaces and we added that as a hack for CJS that only have a “default” export (ie. module.exports = aThing).

thheller09:01:07

so you only use :default in places where you’d otherwise directly invoke the :as

juhoteperi09:01:27

Yeah, but this also affects other functions, in addition to directly exported function

juhoteperi09:01:27

e.g. (:require [react-dom :as react-dom]) (react/render ...) needs to call module$path$react-dom$react-dom["default"].render(...)

thheller09:01:51

hmm that doesn’t do the default no?

juhoteperi09:01:55

or should React-dom and other CJS modules be used with :default react-dom?

thheller09:01:17

aren’t calls like that rewritten as module$path$react-dom$react-dom.render(...)?

thheller09:01:32

since it calls exports.render?

juhoteperi09:01:49

Closure will export the CJS exports object as default property always

thheller09:01:08

I thought only in cases where you directly assign module.exports?

thheller09:01:20

not when using module.exports.foo

juhoteperi09:01:21

no matter if CJS exports object or function, it will always be the default property

thheller09:01:47

are you sure? last time I tested that was only for directly assigned module.exports

thheller09:01:48

oh wow you are correct, it always writes to default

thheller09:01:03

that doesn’t match ES6 at all. strange. (or babel for that matter)

juhoteperi09:01:10

I think it is to make interop between CJS and ES6 easy, inside Closure

juhoteperi09:01:43

Our solution is special case as we do the module processing as separate step and then use the processed JS as input files, instead of doing everything in one go

thheller09:01:44

hmm indeed. I thought the intent was to make CJS look like ES6 and emulate the “default” export since that only exists when directly assigning module.exports not with exports.foo

thheller09:01:12

wonder how rewriting everything to module$path$react-dom$react-dom.default instead of module$path$react-dom$react-dom makes anything easier 😛

juhoteperi09:01:26

it is for ES6 -> CJS interop

juhoteperi09:01:55

Closure rewrites all ES6 to access default properties, so CJS modules need to export that

juhoteperi09:01:26

So they don't need to check on ES6 rewriting if the used module is CJS or ES6

thheller09:01:54

ah right because you otherwise need to check the exported properties

juhoteperi09:01:14

I think I proposed just doing module$path$react-dom$react-dom.default = module$path$react-dom$react-dom, so CJS modules would work as previously, at some point but that probably had some problems

thheller09:01:45

you could use a different name for CLJS and assign that

thheller09:01:22

CLJS_module$path$react-dom$react-dom = cljs.core.jsRequireInterop(module$path$react-dom$react-dom)

thheller09:01:39

and just emit CLJS_... whenever CLJS accesses anything

thheller09:01:03

avoids messing with the names generated by closure

juhoteperi09:01:18

Hmm that could be easyish fix, as we now write provide/requires anyway

juhoteperi09:01:48

But would still require small change to analyzer/compiler to add those CLJS_ parts

juhoteperi09:01:58

except not, that can be done on :js-modules-index

juhoteperi09:01:40

default property can't be solved using that because the module name on js-modules-index is mangled so default is rewritten to default$

thheller09:01:49

the jsRequireInterop could test if the exported object only contains the .default property and adjust accordingly

thheller09:01:58

you don’t need to emit calls to .default if you wrap it

thheller09:01:18

well only for ES6 where it actually exists

juhoteperi09:01:18

Yeah, I meant that this will solve that

thheller09:01:18

I just add the default$ property on the fly

juhoteperi09:01:41

Ah interesting, I'll test this later today

dnolen13:01:48

@juhoteperi I would test for DCE issues when you try this. Aliasing is sketch-y for DCE or at least it was in the past.

dnolen13:01:35

@vemv it doesn’t seem unusual especially if you apply gzipping

dnolen13:01:32

@juhoteperi given that we know if a :require is ES6 or CJS module, another approach would be to add default via the compiler or at the REPL?

vemv13:01:00

thanks for the input David! had forgotten about serving .gz assets

thheller14:01:01

the JS world keeps coming up with new fun hacks … https://twitter.com/dan_abramov/status/950689118077571072

rauh14:01:55

Ugh. Available in CLJS v0.0.1 with macros.

mfikes16:01:28

Opinion... is it a bug that (count (subvec [1 2] 0 1.5)) evaluates to 1.5? (Clojure appears to round down in this case.)

dnolen16:01:56

hrm probably a bug that it passes through

mfikes16:01:41

This is one of those programs where it is difficult to claim whether it is even correct.

mfikes16:01:36

A practical motivating example is a function that attempts to return the fist half of a vector: (defn half [v] (subvec v 0 (/ (count v) 2)))

mfikes16:01:05

I'll log a JIRA so that more investigation can be done to determine whether it is a valid defect.

dnolen16:01:55

@mfikes I’m thinking subvec should Math.floor its arguments?

mfikes17:01:44

@dnolen Yeah, either that, or apply int to its arguments.

dnolen17:01:55

int is a problem

dnolen17:01:41

or actually, I guess not

dnolen17:01:11

I guess vector max size is in fact 2^32?

mfikes17:01:24

Yeah, int appears to be problematic. For example (int 4294967295) evaluates to -1.

dnolen17:01:24

so yeah int is probably fine

dnolen17:01:51

well int is only a problem if it’s too small for vector size

dnolen17:01:53

but it’s not

mfikes17:01:12

4294967295 is 2^32 - 1

mfikes17:01:04

It is as if it is taking the high bit and doing 2's complement

dnolen17:01:30

yeah but if your elements are 64 bit values, that’s nearly 256gb of RAM for that vector anyway

bronsa17:01:04

are you sure vector max size is 2^32 and not 2^31-1 like on the jvm?

mfikes17:01:04

True. Pragmatically speaking int would work and be faster.

dnolen17:01:28

@bronsa er yeah that’s right, the implementation is exactly the same

dpsutton17:01:58

> However, the maximum length of an array according to the ECMA-262 5th Edition specification is bound by an unsigned 32-bit integer due to the ToUint32 abstract operation, so the longest possible array could have 232-1 = 4,294,967,295 = 4.29 billion elements.

mfikes17:01:07

(int 2147483647) is about as high as we can go

dnolen17:01:26

@mfikes feel free to capture this info in the ticket, I’m ok with int

mfikes17:01:37

OK 👍

juhoteperi18:01:32

Testing the module-name default property thing now. I can append some code to the modules, but I can't find easy way to change the code emitted when invoking functions from those modules.

juhoteperi18:01:27

:js-module-index :name probably shouldn't contain .default because that needs to match with the module provide name

dnolen19:01:40

@juhoteperi any reason you can’t do this in :invoke case in compiler?

juhoteperi20:01:37

Invoke wouldn't affect cases where just accessing a var?

dnolen20:01:51

@juhoteperi why all this aliasing stuff instead of ["default"] ?

juhoteperi20:01:36

I'm not sure at which point I could emit that. At the current location the value being emitted is map, munge maybe turns that into string which is then printed?

dnolen20:01:42

why can’t you emit in :var?

dnolen20:01:04

you know the var came from an ns that can’t be accessed directly

dnolen20:01:17

it’s must be referenced as ["default"]

dnolen20:01:33

my only question is how Closure is handling DCE for this case

dnolen20:01:44

are they just emitting default?

dnolen20:01:02

if that’s true than we should probably disable default munging for anything >= ES5

juhoteperi20:01:04

Advanced compiled Reagent output contains lots of mentions of ["default"]

dnolen21:01:12

are you saying that’s from Google Closure?

juhoteperi21:01:50

This is output from Google Closure, yes. Not sure about DCE at all. But at least they don't munge the name.

dnolen21:01:06

well I think we have to do ["default"]

dnolen21:01:15

I saw in the change log that DCE works for ES6 modules

juhoteperi21:01:32

with DCE here do you mean renaming or which optimization part?

dnolen21:01:33

as well as cross module code motion - so that stuff must work even if ["default"] is present

dnolen21:01:51

DCE is dead code elimination - not renaming

juhoteperi21:01:31

I don't understand how accessing module fns/vars through one additional property would affect that (especially after I rewrite this without the additional if on modules)

dnolen21:01:18

in your patch you alias

dnolen21:01:31

so DCE would have to flow through that, I don’t think it will

dnolen21:01:45

it’s not accomplishing anything either that I can tell

dnolen21:01:55

over just emitting ["default"]

juhoteperi21:01:11

Yes, true. I'll try to understand the compiler part to properly emit default directly.

juhoteperi21:01:14

So something like this maybe:

(if (namespace n)
   (emits (namespace n) "[\"default\"]." (name n))
   (emits (name n) "[\"default\"]"))

dnolen21:01:11

yes looks better

juhoteperi21:01:16

Does var-name check make any sense or is there better way to check if var is from JS module?

dnolen21:01:38

I don’t know a better way off the top of my head at the moment

dnolen21:01:42

but it’s a implementation detail anyhow

juhoteperi21:01:00

at this point var name is resolved to the full module$ so it is not easy to check against :js-module-index

juhoteperi22:01:27

I attached latest patch to the issue. Still seeing warnings about optimization passes, otherwise should be OK now.

juhoteperi22:01:23

Aha! Setting :language-in to :ecmascript-3/5 works, 6 or later doesn't. After this the optimized output is the same size as with 946.

juhoteperi22:01:58

Closure defaults to EcmaScript 2017 now, we should probably set default to 3 or 5 at Cljs side?

juhoteperi22:01:34

(Compiler options page says default for both is es3, though default language-out has been no-transpile)

dnolen22:01:35

@juhoteperi yeah we need to change that, if we could get series of patches to bring us up to-date would definitely push this stuff through on Friday

juhoteperi22:01:15

I included languageIn default change on the Closure update patch

dnolen22:01:32

great thanks

juhoteperi22:01:48

and opened PR on clojurescript-site to update the docs

dnolen23:01:27

@juhoteperi which tickets should I look at in which order to push this through?

dnolen23:01:39

I may take a look tomorrow so people can test this out sooner rather than later