Fork me on GitHub
#cljs-dev
<
2016-09-28
>
anmonteiro13:09:58

@juhoteperi nice work on the GCC module stuff. I’m looking over the patch. looks “easier” than initially thought

anmonteiro13:09:10

although “easier” is relative, I imagine how much work you put into this 🙂

anmonteiro13:09:24

at least the diff is small if that’s some measure

juhoteperi13:09:22

diff is relatively small, but time used per line is quite high 😄

anmonteiro13:09:26

I’ve got a few questions: 1. js-transforms has been changed to do a little less work. is this because all modules now need to be processed in 1 go?

anmonteiro13:09:16

2. did you build the compiler and test it with the circle-color project? does it work with advanced compilation too?

juhoteperi13:09:42

1. Yes it is related to that. Previously each file would have been transformed just before running convert just for that file.

anmonteiro13:09:04

3. in the case of 2., probably externs are not needed anymore for the consumed libraries, or is this not true?

juhoteperi13:09:07

Because all files are converted in one step, all the files need to be transformed before Closure compiler tries to parse them.,

juhoteperi13:09:45

I did build the compiler, but did not try with circle-color yet

anmonteiro13:09:12

gotcha, interested in seeing if that works out ^

juhoteperi13:09:35

1. and js-transforms has not in fact been changed, write-javascript was changed so that transform and convert are run outside of it, by process-js-modules

anmonteiro13:09:27

@juhoteperi hah, I didn’t expand the diff 😄

anmonteiro13:09:31

that’s right

juhoteperi13:09:41

Yep, I was confused myself at first

anmonteiro13:09:56

;; FIXME: Should this contain other module types also?
;; And both from foreign-libs and ups-foreign-libs (cljs-1682)?
;; But probably other module-types can't be parsed by this compiler?
^ I think this is the only point where you have some doubts?

juhoteperi13:09:13

Yes, and the change of convert-js-modules API

juhoteperi13:09:33

But I guess the module-types are mostly internal detail anyway as they depend on Closure compiler

juhoteperi13:09:58

So people probably won't define their own convert-js-modules methods (and it is in implementatio ns)

anmonteiro13:09:26

I don’t understand that last part

anmonteiro13:09:37

what change in the API are you talking about?

anmonteiro13:09:49

doesn’t it look the same for CLJS-Compiler consumers?

juhoteperi13:09:51

I guess calling it API is wrong

juhoteperi13:09:08

Just implementation change, convert-js-module -> convert-js-modules and different parameters

anmonteiro13:09:28

ah I see it now

anmonteiro13:09:34

I didn’t notice the pluralization

juhoteperi13:09:35

It used to be called for each foreign-lib with module-type, now it is called for each used module-type with list of all files with of that type

anmonteiro13:09:57

yeah, I got that

anmonteiro13:09:23

@juhoteperi so if I understand the problem correctly, my feedback for the FIXME I pasted earlier would be to include ups-foreign-libs too, filter by the ones that actually have :module-type and convert them

anmonteiro13:09:33

the problem might be dependency order, maybe?

juhoteperi13:09:02

I don't think we need to care about dep order here

anmonteiro13:09:16

Oh, but this is in the context of :commonjs

anmonteiro13:09:25

I get the problem about other module types now

juhoteperi13:09:45

Ah right, there is problem with amdjs

juhoteperi13:09:55

I think I added comment about that somewhere

juhoteperi13:09:07

amdjs and commonjs should be processed together

juhoteperi13:09:15

so they see each other

juhoteperi13:09:43

what module-types are you testing with?

anmonteiro13:09:57

I’ll build the compiler and test the Circle-color project

anmonteiro13:09:11

at least that’s what I was planning to do

dnolen13:09:25

it might be nice to have a script that downloads the deps for that (so we don’t need to include it) and builds it?

anmonteiro13:09:53

@dnolen the circle-color stuff?

anmonteiro13:09:21

I think @juhoteperi emulated that in the tests, not sure if we actually need it in full

juhoteperi13:09:52

Yeah in tests I just included "minimal" files, I manually selected small parts of the files

juhoteperi13:09:03

I could change it to download the files

juhoteperi13:09:14

The JS code probably doesn't really work, but it can be transformed and checked that transformation creates proper Closure module defs

dnolen13:09:01

@juhoteperi what’s blocking it from working? (maybe I missed that you mentioned this earlier)

juhoteperi13:09:35

The mininal React file contains just some function stubs, no real implementation

dnolen13:09:23

oh right, but I mean didn’t Maria’s version actually work?

juhoteperi13:09:30

Yes that should work

dnolen13:09:46

right so I think that’s preferable as a test since it’s the most comprehensive thing we have

anmonteiro13:09:05

@juhoteperi the circle-color project works with your patch 🎉

anmonteiro13:09:14

testing advanced compilation now

anmonteiro13:09:00

Sep 28, 2016 2:39:14 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /Users/anmonteiro/Documents/github/circle-color/resources/public/js/out/Circle.js:5: WARNING - dangerous use of the global this object
  return React$$module$resources$public$js$libs$Circle.createElement("svg", {width:"200px", height:"200px", className:"center"}, React$$module$resources$public$js$libs$Circle.createElement("circle", {cx:"100px", cy:"100px", r:"100px", fill:this.props.color}));
                                                                                                                                                                                                                                                ^

Sep 28, 2016 2:39:14 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /Users/anmonteiro/Documents/github/circle-color/resources/public/js/out/react.js:4: WARNING - constant module$resources$public$js$libs$react assigned a value more than once.
Original definition at /Users/anmonteiro/Documents/github/circle-color/resources/public/js/out/react.js:1
  module$resources$public$js$libs$react = f();
  ^

Sep 28, 2016 2:39:14 PM com.google.javascript.jscomp.LoggerErrorManager printSummary
WARNING: 0 error(s), 2 warning(s)

anmonteiro13:09:18

generates those 2 warnings, and doesn’t really work in the browser

anmonteiro13:09:37

probably not even related to the module stuff?

anmonteiro13:09:37

I actually get a hard error using advanced compilation with 1.9.229

anmonteiro13:09:53

@juhoteperi actually it was an externs problem… It kinda works with advanced compilation using your patch

anmonteiro13:09:07

I thought externs wouldn’t be necessary though?

juhoteperi13:09:33

Probably depends on if module does something dirty, like dynamically create methods

anmonteiro14:09:17

@dnolen there are now a bunch of new closure warning categories that we should probably consider adding to :closure-warnings https://github.com/google/closure-compiler/wiki/Warnings#warnings-categories

anmonteiro14:09:44

should this be a different ticket or included in @juhoteperi’s patch?

juhoteperi14:09:21

Could be probably done easy in different patch?

anmonteiro14:09:54

maybe, after yours has been committed

dnolen14:09:59

@anmonteiro different patch is going to be better

dnolen14:09:29

@juhoteperi @anmonteiro yes if the JS code is generating methods and then statically using them later - there will be problems

dnolen14:09:01

@anmonteiro out of curiosity how big is the gzipped circle example now?

anmonteiro14:09:18

the compiled artifact, right?

dnolen14:09:23

@anmonteiro also interested in whether there’s actually any improvements to advanced compile time? (i.e. building the tests)

anmonteiro14:09:45

I don’t really know how to serve gzip compression locally

anmonteiro14:09:59

should I just gzip the artifact?

anmonteiro14:09:55

@dnolen wow this can’t be right

anmonteiro14:09:07

the whole thing is 351kb, 85kb gzipped

dnolen14:09:18

actually that seems about right to me? would need to compare with CLJSJS React

dnolen14:09:32

the fact that React can’t work without externs is a bit of a bummer

dnolen14:09:35

externs damage DCE

anmonteiro14:09:03

well, I thought it would be a lot more

dnolen14:09:17

@anmonteiro it ain’t shabby! 🙂

juhoteperi14:09:18

extern inference will reduce the damage as the infered extern would only include really used properties

dnolen14:09:36

@juhoteperi yes for CLJS code

dnolen14:09:52

but we can’t fix the internal DCE issue for React and other libs

juhoteperi14:09:21

Well, maybe that can eventually be fixed on Closure or React side

dnolen14:09:21

still this is pretty cool

juhoteperi14:09:36

(use without exterrns)

anmonteiro14:09:54

CLJS master without <@U061V0GG2>’s patch:
Compile sources, elapsed time: 12941.43335 msecs
Compile sources, elapsed time: 0.135049 msecs
Applying optimizations :advanced to 71 sources
Optimizing 71 sources, elapsed time: 39783.06221 msecs


CLJS master with the patch:
Compile sources, elapsed time: 13317.732659 msecs
Compile sources, elapsed time: 0.194513 msecs
Applying optimizations :advanced to 71 sources
Optimizing 71 sources, elapsed time: 40123.820472 msecs

anmonteiro14:09:12

so not really any improvement there

dnolen14:09:13

@anmonteiro heh, little change - that’s ok

dnolen14:09:22

@anmonteiro oh what memory settings are you using though?

juhoteperi14:09:29

did you run it multiple times? looks small enough to be just error

dnolen14:09:31

I would provide at least 1gb of RAM

anmonteiro14:09:50

@dnolen any easy way to do that in the test script?

dnolen14:09:00

I don’t think so, we should fix that 😛

dnolen14:09:35

@juhoteperi the compilation time is long enough for HotSpot to take effect

anmonteiro14:09:50

hrm actually:

java -server -Xms1g -Xmx2g -cp "$CLJSC_CP" clojure.main "$CLOJURESCRIPT_HOME/bin/cljsc.clj" "$@"

anmonteiro14:09:56

this is bin/cljsc

dnolen14:09:07

then yeah, so much for perf improvements 🙂

dnolen14:09:12

that’s ok though

anmonteiro14:09:22

no change is better than a change for worse 🙂

anmonteiro14:09:46

oh one more thing

anmonteiro14:09:54

I’m going to check the bundle sizes

anmonteiro14:09:05

between different Closure versions

juhoteperi14:09:21

Btw. once interesting change in 20160911 was that input sources (e.g. foreign-libs) can now include source-map

anmonteiro14:09:38

ah makes sense

anmonteiro14:09:43

for JSX stuff for example

dnolen14:09:51

@juhoteperi yes I saw that, I also saw some chatter about source map merging

dnolen14:09:05

but I didn’t investigate further - so annoying that we had to roll our own thing for that

dnolen14:09:17

but I guess we were way ahead of that curve

anmonteiro14:09:42

@dnolen the ClojuTRE talk portion of source mapping was compelling haha 🙂

anmonteiro14:09:52

seemed like a really hard thing

dnolen15:09:11

haha, certainly not the most fun feature to implement

anmonteiro15:09:33

now got 38920.114401 msecs compiling the compiler tests

anmonteiro15:09:00

and the bundle size is unchanged

dnolen15:09:02

@anmonteiro huh, I’ll give the patch a shot too later and see what the work machine says

anmonteiro15:09:19

ah right, you might get better times in that machine

anmonteiro15:09:31

this is a 2 year-old 15” Macbook Pro

dnolen15:09:15

this is awesome work @juhoteperi, thanks! 🙂

anmonteiro15:09:26

I second that ^

juhoteperi15:09:03

We should probably check if it is really Closure-compiler compile which takes 40 sec, or if our optimize function is doing something unncessarily slow

dnolen15:09:44

@juhoteperi we should, though I’m pretty sure the advanced compile time thing only measures Closure time - if it doesn’t we should log that

anmonteiro15:09:46

@dnolen it seems it’s measuring more than that

juhoteperi15:09:49

It measures optimize function call time, which initializes compiler, builds input file lists etc, and creates the source map

juhoteperi15:09:25

Most of the stuff it is doing should be very fast, but I don't know about source map generation

dnolen15:09:04

source map generation for advanced output is not fast

dnolen15:09:15

last I checked that alone might double compilation time

dnolen15:09:25

but we’re not doing that for the test build anyway

dnolen15:09:32

@anmonteiro ah, we should fix that

dnolen15:09:50

meaning we should also measure Closure time only in addition to our whole optimization pass

anmonteiro15:09:21

@dnolen should be a simple change, I’ll open a ticket for that as well

anmonteiro15:09:29

s/change/addition

pat21:09:31

@dnolen (eval-str "42") .... I removed the :statement check from (emit* :constant ..., it works and tests pass. Is it that simple?

dnolen21:09:13

@pat that doesn’t sound like a good idea

pat21:09:26

i didnt think so ha

dnolen21:09:51

first I would come up with a real goal see if co-aligns with issues we already know about and then ask questions on how to accomplish that goal

dnolen21:09:05

my impression is you want to be able to do simple scripting a la Clojure with ClojureScript

dnolen21:09:35

(where you don’t need explicit ns forms etc.)

pat21:09:42

Yes mostly already works, just trying tounderstand edge cases

dnolen21:09:51

“mostly already works"

dnolen21:09:11

as I said this is very non-trivial task

dnolen21:09:16

and not newbie friendly at all

dnolen21:09:39

however if you really want to understand what all needs to be done, I’m more than happy to walk you through it

pat21:09:47

let's do it

dnolen21:09:17

so first off we’re not interested in solving this for any particular usecase

dnolen21:09:25

i.e. simple scripting isn’t something we care about in and of itself

dnolen21:09:51

however we would like to get more portable semantics with Clojure - this is the real and only goal

dnolen21:09:16

things like deps.cljs, data literal files, simple scripts all share common need for ns-less files

dnolen21:09:26

in Clojure of course it’s an illusion

dnolen21:09:35

there is a default ns - the user ns

dnolen21:09:19

so we would like for this to work but there’s a serious problem which needs to be addressed first

dnolen21:09:30

how can we make it work for Google Closure compiler

dnolen21:09:37

you cannot have one logical ns splits across multiple files

dnolen21:09:14

this means we need fairly serious compiler support to preserve the desirable semantic

dnolen21:09:39

and we also need to impose constraints on what is permissible so that the work and user expecations are both reasnoable

dnolen21:09:15

for example - we would like require etc. to work

dnolen21:09:40

but it’s desirable to restrict their occurrence to the top of the file

anmonteiro21:09:16

@dnolen just remembered, still awaiting feedback on the comment there ^ (I had forgotten about it, sorry)

pat21:09:33

ok I will study the closure code then and try to get a grip on things

dnolen21:09:13

@anmonteiro responded I think the thing that I’m getting stuck on is in my new comment

dnolen21:09:48

@pat as you can tell we’re in the very deep end - which is why no one has worked on it 🙂

anmonteiro21:09:53

@dnolen I think a :top or :toplevel key in the AST would probably be easier

anmonteiro21:09:59

as per your comment, as opposed to my proposal

dnolen21:09:08

@anmonteiro that sounds reasonable too - feel free to leave that comment

anmonteiro21:09:38

wrt. your question, don’t we have something we use for REPLS that’s :merge?

anmonteiro21:09:04

wouldn’t that work for files without NSes to see each other?

anmonteiro21:09:25

maybe it serves a completely different purpose and I didn’t get it right 🙂

dnolen21:09:35

you’re right that we can probably hack this to work for dev time without issue

dnolen21:09:04

I guess the only issue is compile time where we have to create the cljs.user file

dnolen21:09:18

we need to be clear that side-effecting requires cannot be depended upon

dnolen21:09:29

(when compiling a production thing anyway)

anmonteiro21:09:37

@dnolen when is cljs.user created now?

dnolen21:09:48

it never is - it’s only a REPL thing

dnolen21:09:17

well I mean technically it’s set in the compiler

dnolen21:09:24

but nothing is around it to make it actually work

anmonteiro21:09:38

so proposal: we create it upon seeing the first file without a NS statement

anmonteiro21:09:04

if we never see one, things work as before (currently)

dnolen21:09:06

oh hrm …….

dnolen21:09:11

here’s an interesting idea

dnolen21:09:20

so you can use multiple provides in a Closure file

dnolen21:09:32

cljs.user.file1

dnolen21:09:35

cljs.user.file2

dnolen21:09:00

and we can have a custom resolve for cljs.user to look in these sub namespaces to resolve a var

dnolen21:09:08

then everything should just work - hunky-dory

anmonteiro21:09:26

oh you might have mentioned this before

dnolen21:09:33

I don’t think so

dnolen21:09:38

feels like a new idea! 🙂

dnolen21:09:48

but maybe you’re right

dnolen21:09:56

and I just forgot

dnolen21:09:12

in anycase if we can make Google Closure do all the work - that’s better

dnolen21:09:15

and easier

anmonteiro21:09:31

@dnolen ah no you mentioned “gen a random NS"

dnolen21:09:51

I don’t think it needs to be random

anmonteiro21:09:01

yeah, if we have the file name it could work

dnolen21:09:09

actually yes that would be best

anmonteiro21:09:11

probably also need to take path into account and munge it somehow

dnolen21:09:15

cljs.user.foo_file

dnolen21:09:33

though we’ll still need to disambiguate

dnolen21:09:43

since multiple deps.cljs and data literal files is a thing

anmonteiro21:09:17

yeah but they’ll probably be at different paths in the classpath, right?

dnolen21:09:29

actually they won’t be

dnolen21:09:37

deps.cljs and data literals files need to be at the top

dnolen21:09:47

definitely deps.cljs

anmonteiro21:09:06

so there would be no way of mapping errors to those files?

dnolen21:09:28

it should just work

anmonteiro21:09:36

maybe that’s wrong, we probably know which file "we’re compiling right now"

dnolen21:09:49

I don’t think we need to do anything at all

dnolen21:09:56

as long we generate closure namespaces

dnolen21:09:03

the only trick here is the custom var resolution

dnolen21:09:31

so this would be simpler than my other previous suggestions

dnolen21:09:44

source mapping, advanced compilation, dev time - it should all just work

anmonteiro21:09:09

@dnolen I’ll capture all this in a comment to the issue, but there’s something I’m not getting still

anmonteiro21:09:44

if we’re analyzing a file with an NS statement, do we use the :toplevel thing?

anmonteiro21:09:51

the name might be misleading to me

anmonteiro21:09:07

did you mean it only in the context of files without NS or every top-level declaration??

dnolen21:09:16

this is why I suggested putting top level into :env

dnolen21:09:24

and discarding it anywhere recursive

dnolen21:09:37

analyze-seq yada

anmonteiro21:09:29

@dnolen just to make sure I understand correctly: we’d assoc :toplevel true in the analyzer entry points (e.g. analyze-file) but remove it from the env in the recursive calls?

dnolen21:09:51

and if we are in parse ‘ns it better be :top-level

dnolen21:09:09

and we also need to add specials forms for require etc.

anmonteiro21:09:23

right, no nested ns statements

dnolen21:09:41

and these special forms also check for :top-level

anmonteiro21:09:05

so then we could relax the assumption that every file needs ns

anmonteiro21:09:00

this first part doesn’t seem hard to implement at all at first sight

dnolen21:09:07

we will need some compiler voodoo

dnolen21:09:24

if the first form isn’t ns we need to emit the code for declaring a cljs.user sub-ns

anmonteiro21:09:38

ah! that’s what I was missing

anmonteiro22:09:24

@dnolen added a comment to the issue, let me know if anything doesn’t look quite right; I might have misunderstood something http://dev.clojure.org/jira/browse/CLJS-1346?focusedCommentId=43974&amp;page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-43974

anmonteiro22:09:38

this looks promising! I’ll only be partially happy when we have support for data_readers.cljc!

dnolen22:09:39

@anmonteiro summary looks good to me

anmonteiro22:09:52

awesome, thanks

anmonteiro22:09:13

looks like a fun weekend project 😉

anmonteiro22:09:30

@dnolen: now that I think about it, what did you mean by :top? Top-level expressions or top of the file?

anmonteiro22:09:48

I might have misunderstood what you meant by top

dnolen22:09:55

will need to be two separate things

dnolen22:09:05

top-level expression is one thing - top of the file another thing

dnolen22:09:42

the second one is a bit simpler, just some state for analyzing/compiling files

anmonteiro22:09:01

Yeah, but what do we want to do with toplevel expressions beyond disallowing ns, require and such?

dnolen22:09:44

that’s it

anmonteiro22:09:46

Or rather, only allowing them at the toplevel

anmonteiro22:09:03

So I suppose top of the file is not really needed since it'll just be the first form we find

anmonteiro22:09:11

Like it's done now

dnolen22:09:09

right the first form logic stuff should be simple

dnolen22:09:21

it’s just the hack we need in the compiler for emitting the first form, but even that should be ok

anmonteiro22:09:59

Are you talking about something that's already in place or?