<< Back to previous view

[CLJS-798] regression: upstream dependencies are no longer honored Created: 16/Apr/14  Updated: 17/Apr/14

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Toby Crawley Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: File restore-upstream-deps.diff    

 Description   

The latest cljs no longer honors upstream deps. This is a regression introduced by https://github.com/clojure/clojurescript/commit/e16a3da. The js-dependency-index is now calculated once, before the upstream deps are loaded. One potential fix is to regenerate the index after loading the upstream deps, but I'm not familiar enough with the compiler to know if that's a bad idea. I'll attach a patch with that change for review.



 Comments   
Comment by David Nolen [ 17/Apr/14 11:03 AM ]

Out of curiosity are you relying on this functionality and in what way?

Comment by Toby Crawley [ 17/Apr/14 11:48 AM ]

I currently use a deps.cljs in https://github.com/vert-x/mod-lang-clojure - it provides a cljs library that relies on two javascript libraries, and I want users to be able to use my cljs library without having to specify its upstream js deps as configuration to their cljs build. If there is another way to achieve that, I'm open to suggestions, especially given that deps.clj is experimental.





[CLJS-792] cljs.core.reducers/reduce does not support cljs.core/PersistentArrayMap Created: 07/Apr/14  Updated: 18/Apr/14

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Shahar Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File CLJS-792.diff    

 Description   
((require '[clojure.core.reducers :as r])
(into [] (r/map identity {}))
;; Clojure: []
;; ClojureScript: Error: No protocol method IReduce.-reduce defined for type cljs.core/PersistentArrayMap: {}





[CLJS-787] cljs.reader does not read blank string as nil Created: 20/Mar/14  Updated: 18/Apr/14

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File CLJS-787.diff    




[CLJS-755] Expose Clojure's hash calculation helper API Created: 29/Jan/14  Updated: 29/Jan/14

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

To aid in collection portability between Clojure implementations.






[CLJS-754] Assess Murmur3 for Collections Created: 29/Jan/14  Updated: 26/Feb/14

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

We should assess the performance implications of adopting Murmur3 for hashing - http://github.com/clojure/clojure/commit/dff9600387b962f16fc78e6477e10e34651fd366



 Comments   
Comment by Francis Avila [ 19/Feb/14 5:19 AM ]

I have a (lightly tested, probably untrustworthy) gist of clj.lang.murmur3 ported to javascript. The two other murmur3 js libs I saw both had flaws which I attempt to address.

Found a jsperf comparing murmur3 libs and added a goog.string.hashCode case as a baseline. This jsperf test isn't very good because:

  1. it only tests a tiny string
  2. the murmur libraries are both flawed:
    1. one of them assumes strings are 8-bit only (consumes 4 chars at a time for hashing)
    2. the other one treats strings as utf-16, but does not account for integer overflows.

However, the results are still informative--string hashing with murmur3 in js will probably be half as fast as goog.string.hashCode, except on firefox where it is about the same.

Not nearly enough data for a true assessment, but I hope it helps.

Comment by Francis Avila [ 19/Feb/14 10:20 AM ]

It occurs to me that the slowdown is probably entirely due to integer multiplication, and that spidermonkey's jit is detecting and optimizing that pattern of code to Math.imul while the others are not. Math.imul is still experimental but supported on many browsers already--it might be better to polyfill it instead of unconditionally inlining the bit-shifting. Will try later.

Comment by David Nolen [ 19/Feb/14 2:23 PM ]

There's very little need to write this logic in JavaScript - it should just be done in ClojureScript. I think modern JS engines can probably handle Murmur3, here are some perf numbers for simulating multiplication of 2 unsigned 32bit ints that I found on StackOverflow http://jsperf.com/bit-multiply

Comment by Francis Avila [ 19/Feb/14 3:14 PM ]

I agree re: implementing in clojurescript. It looked hairier when I started.

You're also right about the bit multiply. I extended that case with larger input values and a comparison against Math.imul and it seems to be compiling to the same code. Maybe inlining the bit-multiply is confusing the jits, or maybe it's something else entirely. I'll give it another shot when I get a chance.

Any advice on setting up tests for this that would work in the clojurescript project's environment? Probably what we want is a generative test which ensures the hash value in cljs is the same as in clj?

Comment by David Nolen [ 19/Feb/14 4:34 PM ]

Generative tests that guarantee the hashes are the same in both CLJS & CLJ is a nice to have and not really a priority. Murmur3 is pretty straightforward looking, I'm sure we can get it right

Comment by David Nolen [ 20/Feb/14 9:57 AM ]

One of the V8 engineer did a more accurate version of the benchmark. It looks like Math.imul is the way to go and we should shim in something for older browsers.

http://jsperf.com/bit-multiply/5

Comment by Francis Avila [ 22/Feb/14 2:20 AM ]

I thought those results looked too good. 32-bit maths in js is always tricky, hence desire for tests, especially if one of the use cases is comparing compile-time and run-time hashes.

Cleaned up js implementation of murmur3 just to get something up quickly to assess murmur3 performance. It uses Math.imul or a shim for integer multiplication. Then created a pair of jsperfs:

In both tests, setup code is the same: pretty-printed closure-advanced-compiled code from my js-murmur3 implementation, and copy-pasted code from cljs's current number and string hashing.

Results are...interesting. So interesting that I am suspicious that something is wrong with my benchmarks or code. Perhaps you can have Mr. Egorov take a look? In summary:

  • murmur3 int hash is about an 8% performance regression in firefox and safari.
  • murmur3 string hash is more than double the speed of goog.string.hashCode on ff and safari for small strings, and even better for large ones.
  • Except in chrome, where murmur3 is abysmal--about an order of magnitude regression on both ints and strings!

All these browsers have a native Math.imul, and Chrome's imul is definitely working. There must be something else making chrome's jit cranky.

I did not expect murmur3 to perform so well or so badly!

Comment by Francis Avila [ 26/Feb/14 3:37 AM ]

Maintaining a fork with murmur3 hashing (see murmur3 branch). So far numbers, strings, and collections (ordered and unordered) hash identically to clojure 1.6-beta1, but symbols and keywords do not. I suspect integer-math differences in cljs's hash-combine vs clojure.lang.Util/hashCombine, but I have not isolated the problem yet.

Comment by Francis Avila [ 26/Feb/14 3:06 PM ]

Nevermind, cljs and clj reverse the order of ns+name hashing and I just didn't notice.

This branch appears to produce hashes identical to clojure 1.6 for the following types:

  • strings
  • keywords and symbols
  • vectors, lists, persistentqueue, seqs
  • maps, sets
  • integers representable with doubles in js (i.e., about 53 bits of signed integer)

Types which do not hash the same (likely incomplete list):

  • uuids
  • instances
  • floating-point types. Clojure 1.6 still uses Java's native hashCode for these, and checking for a non-int is a bit involved in js, so I just hash all js numbers as if they were longs. This might be a bad idea if we have a collection of "real" doubles.




[CLJS-736] Functions folder and reducer broken for types nil and array + fix for typo Created: 29/Dec/13  Updated: 18/Jan/14

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Jonas De Vuyst Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-736-alt.patch     Text File CLJS-736.patch     Text File CLJS-736-patch-1-redux.patch     Text File CLJS-alt-satisfies.patch    
Patch: Code and Test

 Description   

1. This currently doesn't work:

(->> nil
(r/map identity)
(r/reduce + 0))
; org.mozilla.javascript.JavaScriptException: Error: No protocol method IReduce.-reduce defined for type null

The reason for this is that reducers created by r/reducer or r/folder, invoke -reduce (of IReduce) directly. They thereby bypass the special case for nil in the function r/reduce.

2. An entirely analogous problem exists for collections of type array.

3. The patch CLJS-700 mistakenly defined coll-fold for the type cljs.core/IPersistentVector. This should have been cljs.core/PersistentVector. (There exists no protocol IPersistentVector in ClojureScript.)

I will shortly attach a patch that addresses all of the above problems by implementing IReduce for nil and array. The patch also includes unit tests.



 Comments   
Comment by Jonas De Vuyst [ 29/Dec/13 2:22 PM ]

Alternative patch in which r/reduce and r/fold treat arrays and nil as special cases – as opposed to having arrays and nil implement IReduce and CollFold.

The functions r/reducer, r/folder, and the protocol methods of r/Cat now call r/reduce and r/fold instead of calling -reduce and coll-fold directly.

This patch also fixes a bug in the coll-fold implementation for Cat, which previously used (reducef) as the initial value rather than (combinef). The new code is copied and pasted from the Clojure implementation and uses the fork-join stubs.

Comment by David Nolen [ 30/Dec/13 8:23 AM ]

The implements? should probably be a satisfies? in the second patch. Have you run any benchmarks of before/after the patch?

Comment by Jonas De Vuyst [ 30/Dec/13 11:24 AM ]

If I understand correctly then (satisfies? x y) is roughly equivalent to (or (implements? x y) (natively-satisfies? x y)).

If native types (nil, array, object currently) are treated as special cases then implements? seems more appropriate.

satisfies? works also, however, so I have attached a new 'alt' patch.

Comment by Jonas De Vuyst [ 30/Dec/13 11:26 AM ]

The first patch is in fact faster when running the following code:

(time (->> (repeat 1000 (vec (range 1000)))
vec
(r/mapcat identity)
(r/map inc)
(r/filter even?)
(r/fold +)))

This takes about 700 msecs. Using the first patch this terminates 100-300 msecs faster. This is after repeated (but informal) testing.

I guess the worry is that the first patch would slow down other random code since it involves extending the types nil, array, and object. I'm not sure what exactly I should test for though.

(Note that the 2nd and 3rd patch also contain a fix for Cat and include more unit tests. The first patch should preferably not be applied as-is.)

Comment by David Nolen [ 30/Dec/13 11:35 AM ]

Yeah you're timing too many things, including vec, range, lazy sequences. Also testing a small N. Take a look at the reducers example on the Mori README - https://github.com/swannodette/mori. Thanks.

Comment by Jonas De Vuyst [ 30/Dec/13 12:52 PM ]

I tried running the following code:

(let [coll (vec (repeat 1000 (vec (range 10))))]
  (time (doseq [n (range 1000)]
               (->> coll
                    (r/mapcat identity)
                    (r/map inc)
                    (r/filter even?)
                    (r/fold +)))))

Some of the last results I got were:

1st patch: 75680 msecs
2nd patch: 76585 msecs

Truth be told, although the first patch seemed to win most of the times, sometimes the second patch was faster.

One other thing I tried was removing the implements?/satisfies? check from the second patch and overriding the protocol method coll-fold for the type object instead (as in the first patch). This 'hybrid' approach generally (but not always) seemed to result in a slowdown.

I'm not sure how I should proceed. Should I perhaps just run both patches simultaneously for several minutes?

Comment by David Nolen [ 30/Dec/13 1:21 PM ]

This is still a bad way to do timing, you're recording the cost of range and seq'ing. Use dotimes.

Comment by Jonas De Vuyst [ 30/Dec/13 4:33 PM ]

Hm. I guess the lazy sequence does lead to a lot of allocations.

Alright, I rewrote my test and ran it a few more times. I now also tested on both vectors and arrays.

Patch 1 needed a slight tweak. When coll-fold is invoked, patch 1 only specifies a fallback for type object (i.e. r/reduce is called). I had to add the same fallback for type array. (This is weird!)

So here are the results.

For vectors:

(let [coll (vec (repeat 100 (vec (range 100))))]
  (time (dotimes [n 3000]
          (->> coll
              (r/mapcat identity)
              (r/map inc)
              (r/filter even?)
              (r/fold +)))))

Patch 1: 205872 msecs
Patch 2: 210756 msecs

For arrays:

(let [coll (into-array (repeat 100 (into-array (range 100))))]
  (time (dotimes [n 3000]
          (->> coll
              (r/mapcat identity)
              (r/map inc)
              (r/filter even?)
              (r/fold +)))))

Patch 1: 123567 msecs
Patch 2: 119704 msecs

I ran my tests a few times and the results were pretty consistent. Patch 1 is faster for vectors and patch 2 is faster for arrays.

This makes sense.

In patch 1 reducer will call -reduce directly. In patch 2, reducer first calls r/reduce, which calls -reduce if the collection is a vector and array-reduce if it's an array. Hence patch 2 contains an extra function call in the case of vectors, but avoids invoking a protocol method on a native type in the case of arrays.

Using macros (or copy and paste) the extra function call can be avoided. Would that be worth trying or is it more important to keep the code clean?

I just realized that patch 2 is semantically slightly different from what Clojure does, although perhaps this is a bug in Clojure: <https://groups.google.com/forum/#!searchin/clojure-dev/kv-reduce/clojure-dev/bEqECvbExGo/iW4B2vEUh8sJ>. My suggestion to use a macro (or copy and paste) to avoid the extra function call in patch 2, could also fix this discrepancy.

Comment by David Nolen [ 30/Dec/13 4:42 PM ]

How are you benchmarking this? With V8? JavaScriptCore? SpiderMonkey? In the browser? What optimization settings, etc.

Comment by Jonas De Vuyst [ 30/Dec/13 4:48 PM ]

I used repljs (Rhino?). I'll test again in a more realistic setting tomorrow.

Comment by David Nolen [ 30/Dec/13 4:54 PM ]

Yeah, benchmarking with Rhino isn't informative.

Comment by Jonas De Vuyst [ 31/Dec/13 1:40 AM ]

I compiled the same code (with n=3000) using cljs with "{:optimizations :advanced}".

I then tested it in the latest stable releases of Firefox, Chrome, and Safari. I closed all my browsers. For each browser I then followed the following procedure:

  • Open the browser
  • Open the developer console
  • Run the benchmark for patch 1
  • Run the benchmark for patch 2
  • Run the benchmark for patch 1 and write down the result
  • Run the benchmark for patch 2 and write down the result
  • Close the browser

Firefox:

  • Patch 1. Vectors: 26057 msecs
  • Patch 1. Arrays: 25026 msecs
  • Patch 2. Vectors: 26258 msecs
  • Patch 2. Arrays: 36653 msecs
  • Summary: Patch 1 is faster for vectors and arrays

Chrome:

  • Patch 1. Vectors: 7804 msecs
  • Patch 1. Arrays: 7092 msecs
  • Patch 2. Vectors: 7754 msecs
  • Patch 2. Arrays: 6768 msecs
  • Summary: Patch 2 is faster for vectors and arrays

Safari:

  • Patch 1. Vectors: 167230 msecs
  • Patch 1. Arrays: 108780 msecs
  • Patch 2. Vectors: 173940 msecs
  • Patch 2. Arrays: 110012 msecs
  • Summary: Patch 1 is faster for vectors and arrays

I'm not sure what to make of this.

Comment by Jonas De Vuyst [ 31/Dec/13 2:47 AM ]

I have attached a new version of the first patch.

This patch fixes an issue with r/Cat. (This issue was also addressed in the second and third patch. A unit test is included.).

This patch also fixes r/fold for arrays.

To summarize, a choice needs to be made between the following patches.

  • CLJS-736-patch-1-redux.patch
  • CLJS-736-alt.patch (uses implements?) / CLJS-alt-satisfies.patch (uses satisfies?)

The implementation details are patch-1-redux is more similar in spirit to the Clojure source code. The alt patches are more similar in spirit to the ClojureScript source code.

As explained above, the alt patches are semantically a bit different from the original Clojure source—but it's not clear which behavior is 'right'.

Comment by David Nolen [ 16/Jan/14 5:27 PM ]

The benchmarks would be more informative if they explained the performance before and after that patch.

Comment by Jonas De Vuyst [ 18/Jan/14 11:55 AM ]

r/reduce previously didn't work for nil or JavaScript arrays.

One reason why I have trouble recommending a patch is that I don't know what use case you would like to optimize for.

Comment by David Nolen [ 18/Jan/14 12:30 PM ]

Yes but now that we have new logic we can at least test the lack of regression on the other types.

Comment by David Nolen [ 18/Jan/14 12:40 PM ]

Ok I tried to apply this patch and run ./script/benchmarks in the repo but the patch will no longer apply. Can we rebase the patch on master. Thanks. If you also want to give the benchmarks a shot follow these instructions to install the JS engines - http://github.com/clojure/clojurescript/wiki/Running-the-tests. Then you can also run the benchmarks at the command line. I see there aren't any reducers benchmarks, I will add some.





[CLJS-712] resolve-var for symbol with dot still wrong Created: 03/Dec/13  Updated: 03/Dec/13

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

We need to recur on the first segment passing an new additional argument to resolve-var indicating that we should not try to resolve in the current namespace and instead warn.






[CLJS-710] port clojure.pprint Created: 02/Dec/13  Updated: 28/Jan/14

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None





[CLJS-705] locals clearing Created: 27/Nov/13  Updated: 29/Nov/13

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Without locals clearing ClojureScript is likely to suffer from the same cases as Clojure did for common uses of lazy sequences.



 Comments   
Comment by David Nolen [ 29/Nov/13 3:03 PM ]

For this we'll need to introduce some special private way to set a local to nil, i.e. (_clear_local sym)





[CLJS-666] :require-macros should throw a sensible error if no macro file exists Created: 06/Nov/13  Updated: 06/Nov/13

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None





[CLJS-656] Look for goog-style JavaScript dependencies on the classpath automatically Created: 01/Nov/13  Updated: 17/Mar/14

Status: In Progress
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File CLJS-656.diff    
Patch: Code

 Description   

Right now, only actual goog.* JavaScript dependencies (as shipped w/ GClosure or in its "third-party" library and specified in its deps.js file) are available via (:require ...), etc.; other dependencies that contain goog.provide "declarations" can only ever be loaded via :libs and the classpath scanning (or direct filesystem reference) it provides.

This should change so that any referenced Closure-safe JavaScript dependencies may be resolved from their expected location on the classpath. e.g. (:require foo.bar) would resolve to a foo/bar.js resource on the classpath, which presumably would be required to contain a goog.provide("foo.bar") declaration.

With this change, :libs would be downgraded to support only direct references to filesystem files. (Q: should the option be renamed to reflect this, and to provide an easy configuration validation check for people that are using :libs as it exists now?)

This would be a breaking change to downstream tooling.

(The above is a summary of #clojure discussion with David Nolen.)



 Comments   
Comment by Chas Emerick [ 01/Nov/13 3:06 PM ]

This would eliminate the need for CLJS-526

Comment by Michael Alyn Miller [ 01/Nov/13 11:27 PM ]

I would really like to see this change. I am building a ClojureScript wrapper around a JavaScript library (which I GClosure-ified) and the only real way to do this at the moment is with your {{:libs [""]}} trick. That works, but it also pollutes the project.clj of every consumer of the library.

Comment by David Nolen [ 08/Nov/13 12:26 PM ]

Related information, it appears I was wrong about GClosure following Java classpath convention for layout. In order to resolve the location of Google Closure Library files, closure.clj parses deps.js in the Google Closure Library JAR.

Still I think we should move forward with the approach outlined here.

Comment by Chas Emerick [ 08/Nov/13 2:25 PM ]

My current understanding is that deps.js is an optional delivery mechanism, and perhaps is relatively unused outside of GClosure itself. I still need to dig into the GClosure packaging guidelines, probably won't happen pre-Conj; until then, I can't add much except that I also much prefer the current proposal than attempt to find, consume, and produce these deps.js files. It's not as if people are banging down the door for GClosure-packaged library compat, etc.

Comment by David Nolen [ 08/Nov/13 2:37 PM ]

Pretty much summarizes my thoughts as well.

Comment by Brenton Ashworth [ 11/Nov/13 9:35 AM ]

In my opinion this is a good change. What follows is some context that I have on the purpose of the :libs option. I hope this information is both accurate and helpful.

At the birth of ClojureScript it was not clear how much we would depend on the Java classpath. I think it has become clear that the compiler is typically used as a library which has access to the project's classpath. We should embrace this and make it more friendly to use in this environment.

In the first version of ClojureScript, the compiler could compile a file without resolving dependencies. Problems with deps where either found at run time or when files are handed to the GClosure compiler for compilation. This may no longer be true. I am ashamed to say that I haven't looked at the source in a while. It is important to remember that GClosure deals only with JavaScript sources. Once all ClojureScript sources have been compiled to JavaScript, a set of files is handed to the GClosure compiler for optimization. The purpose of the `:libs` option is to allow any other JavaScript sources which are GClosure compatible, and upon which the sources depend, to be found and handed to the GClosure compiler.

In the beginning, we added GClosure support (dealing with JavaScript files) on top of the CLJS compiler. This change would allow the CLJS compiler to contribute useful information to the GClosure compilation process and remove the need to specify known information elsewhere.

Comment by Chas Emerick [ 12/Nov/13 5:11 AM ]

Thanks for the background, Brenton. Assuming you'll be at the Conj, I may have a couple other more minor questions for you.

I think the results from the (still ongoing) community survey will further support the use of the classpath as a reasonable option easily available from the tools people are actually using.

I've dug around the GClosure library docs w.r.t. their build tools, and they seem uniformly directed at people building applications; outside of supporting application use cases, I don't see any discussion of distributing the artifacts of e.g. closurebuilder.py & co. as libraries. A (hardly comprehensive) search of libraries distributed with deps.js files yielded nothing; in fact, the only mentions of deps.js that I can find on github are in .gitignore s and HTML pages of app repos, and in files related to build processes that are reading deps.js shipped with GClosure and its third-party-library.

So, I think we can safely not think about deps.js again, except for the special case of the upstream GClosure libs, which is already taken care of.

One piece of "prior art" of which I was not fully cognizant until yesterday (!!) is this:

https://github.com/emezeske/lein-cljsbuild/issues/143

Essentially, cljsbuild implicitly sets up e.g. /closure-js/libs as a classpath :libs root, same for /closure-js/externs. I've not tested this myself yet, but others apparently have had success. Seems like a very reasonable approach, absent something built-in to the compiler. Externs are certainly out of scope here, but some discussion of what can be done to similarly ease their use is warranted; perhaps adopting a variant of cljsbuild's approach…

Comment by Chas Emerick [ 15/Feb/14 8:31 AM ]

I've started working on this in earnest. It's turning out to be more complicated and delicate than I expected. I see no reason why it won't be resolved successfully as discussed previously, but I thought I'd show my face here so people know that progress is being made.

Comment by Thomas Heller [ 16/Feb/14 3:14 AM ]

I implemented a different approach to this problem in https://github.com/thheller/shadow-build. It has been working well for me so let me explain what I do.

1) The user specifies all source paths
https://github.com/thheller/shadow-build/blob/master/dev/build.clj#L53-55

Each source path is scanned for .cljs and .js files. JS files are just scanned for goog.provide/require statements via regexp, CLJS files assume the NS form is first and is scanned as such to extract the require/provides. No further analysis is done.

2) Out of all this information a dependency graph is built (plus a lookup index, eg. where can I find goog.string).

3) The user specifies his "main" namespaces. A main is simply a namespace that either exports functions for outside use or runs code on its own behalf. Those main namespaces are sorted in topological order so we have the entire graph needed for the compilation. If a namespace is not found required but not provided I do not attempt to build anything but throw an exception.

4) All files are "compiled" in reverse order. Since only CLJS requires compilation, JS files are skipped. CLJS compilation is also a lot "safer" than cljs.closure since dependencies are "guaranteed" to exist (eg. can't compiled a file we don't have all requires for, which may or may not be ideal).

5) Optionally everything may be grouped into modules and either flushed unoptimized and fed to the closure optimizer and then output to disk.

Although I must admit I never used the ```:libs``` directive I'm pretty sure its not needed by using this approach, just put it into a "source path" and let the regexp scan it. As long as it has goog.provide/require its good to go. You can even make it a "main" by just specifying its goog.provide, not required to make a CLJS main.

I've been pretty busy lately and shadow-build is missing some cleanup I want to do but its working very well for my project (> 10k CLJS LOC).

Maybe some of the work I did could help here.

Comment by Chas Emerick [ 17/Feb/14 9:51 AM ]

Thanks, Thomas. I haven't looked at shadow-build much before, but I've stuck a pin in it for further review. The biggest complication of this change is not the primary objective, but my wanting to accomplish it with minimal impact on cljs.closure. There are many things that I would like to change there, but absent an effective set of tests, any simplifying refactor is fairly risky. The first step to accomplishing that would probably be to invert the existing dataflow to produce a (testable) build plan instead of actually building anything. But again, work for another day.

Anyway, I think I have things pretty well settled out locally, though there's some policy decisions to be made. (Putting those in a separate comment.) My biggest hangup was stepping on some helper functions that were being used by the externs stuff in ways I wasn't aware.

Comment by Chas Emerick [ 17/Feb/14 11:34 AM ]

Progress so far on this is available @ https://github.com/cemerick/clojurescript/tree/CLJS-656

Changes on that branch:

  • goog-style JavaScript libraries are no longer resolved via classpath scanning (though it is still used for loading externs and such)
  • I've replaced the system-property-based classpath discovery with one that looks at the actual classloaders in use; this should avoid all sorts of confusing/incorrect behaviour when the compiler is being used in environments where the effective classpath is dynamic.
  • JavaScript dependencies are resolved in one of two ways:
  • resolution through the goog deps.js, the consumption of which remains untouched (though it looks like the classpath-scanning mechanism yields effectively equivalent results as reading deps.js, so we could probably eliminate goog-dependencies entirely in the future…)
  • direct lookup on the classpath based on the "namespace" being :require}}d; e.g. {{(:require foo.bar) will look for a foo/bar.js resource on the classpath, which must contain a corresponding goog.provide("foo.bar") declaration

This all works nicely with the couple of libraries I have GClosure-ified (pprng is the only public instance of that at the moment).

As noted in a comment in the WIP change, a problem with the direct-lookup policy is that goog-style JS dependencies that aren't part of the "official" GClosure library can provide one and only one name, the one corresponding to their position on the classpath. Not a problem from my perspective, but it does cut off the practice of libraries providing type ctors or pools of constants directly, e.g. goog.string.Unicode. This is really just a policy question:

Q1: Should ClojureScript support requiring JavaScript namespaces that don't correspond with the naming/location of their source files?

IMO, the answer to that should be 'yes'; saying 'no' means that we either need to prevent such requires w.r.t. official GClosure library, or that that collection of libraries is effectively a special case from the user's perspective. 'Yes' dumps us back to doing classpath scanning to discover goog-style JS dependencies; not a problem IMO (it would make the solution to this issue effectively equivalent to CLJS-526), but something that David said in the IRC conversation preceding this issue's creation should be eliminated. I think that secondary objective preceded the understanding that GClosure lib doesn't use Java-style file layout itself.

Comment by David Nolen [ 17/Feb/14 11:48 AM ]

Just a heads up that this will likely conflict with CLJS-615, which I would like to land first.

Comment by Chas Emerick [ 17/Feb/14 11:52 AM ]

That's fine, this patch will be small either way and not difficult to rebase.

Comment by Thomas Heller [ 18/Feb/14 3:58 AM ]

IMHO the direct lookup is not very useful. Discovering/scanning all JS files it not that expensive and its quite common in Closure to have many provides per file. Not a big fan of "special cases" either, so I think every JS file should be treated equal regardless of Closure-Library or not.

The real problem is that cljs.closure/build is not a very flexible interface and calling it repeatedly (lein cljsbuild auto) causes problems because all of the sudden cljs.closure has to take care of caching, reloading, etc. IMHO something that should probably be left to tooling and not pollute cljs.analyzer/compiler.

Comment by Chas Emerick [ 18/Feb/14 6:42 AM ]

I agree; I don't see any way (or really, good reason) to avoid the classpath scanning without diverging in important ways from user-dev expectations re: how the goog provide/require system works. Now that CLJS-615 has landed, I'll tweak up my changes to reflect that, and apply cleanly to master in a proper patch.

Comment by Chas Emerick [ 18/Feb/14 9:49 AM ]

Patch attached. It does all that I described above, but does not constrain the location of goog-style JavaScript dependencies within the classpath; the goog.provide declarations in such libraries is the sole constraint on their visibility from user-dev code.

FWIW, the classpath scanning takes ~600ms on my machine the first time, and ~7ms after that (resource lookup from the classpath is pretty heavily cached in the JVM), so I don't think we need to worry about it being a problem in compiler performance.

Comment by Thomas Heller [ 18/Feb/14 10:30 AM ]

FWIW I use reducers to bring that 600ms to around 200ms on my machine, should be possible to make that even faster since right now I (and you I presume) open JAR files ONCE to extract all names and then ONCE per URL. Would probably be faster to open/scan once in total.

But like you said, since its done on init it probably is no problem.

Comment by Chas Emerick [ 19/Feb/14 3:48 AM ]

No, we touch the jars each time. You're right that it could be sped up for the first compile very obviously (I just try to keep my patches as slim as possible, so I generally don't touch what I don't need to). I'll see about it in addressing CLJS-770.

Comment by Chas Emerick [ 24/Feb/14 4:30 AM ]

New patch attached that applies cleanly, now that CLJS-770 / cljs.js-deps is merged.

Comment by David Nolen [ 07/Mar/14 2:39 PM ]

After some thought I still think we should special case Google Closure Library. Everything else must use Java classpath conventions. There's just not enough incentive to support Google Closure idiosyncracies when we have simpler and more sensible conventions in ClojureScript itself. The universe of libraries that follow Google Closure idiosyncrasies is not exactly large, and the ClojureScript ecosystem is evolving at a quick clip. Integration of large non-Closure compatible libraries like React is already straightforward due to :preamble. Let's wrap this one up and move on.

Comment by Chas Emerick [ 12/Mar/14 6:25 AM ]

Update forthcoming shortly.

Comment by Chas Emerick [ 17/Mar/14 11:32 AM ]

New patch attached. This one requires that any non-extern libraries loaded via the classpath contain a goog.provide declaration that corresponds to its relative location on the classpath and to the name used for that library in ClojureScript (e.g. in ns forms).

:libs continues to exist, though is now limited to pulling in libraries from the filesystem, and enforces no constraints on goog.provide declarations or relative position on the filesystem.





[CLJS-655] Require main entry point (file) to serve as compilation root Created: 01/Nov/13  Updated: 01/Nov/13

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Per David Nolen in #clojure, a single file should serve as a compilation root, rather than the compiler needing to scan a given directory for files.

This leaves a couple of questions:

1. Are discovered dependencies then only ever loaded from the classpath?
2. If so, what does command-line (i.e. bin/cljsc compilation look like? Force people to export CLASSPATH appropriately, provide a set of alternative source paths via arguments, or other?

This would yield a fair bit of breakage downstream, e.g. cljsbuild.



 Comments   
Comment by David Nolen [ 01/Nov/13 7:10 PM ]

It's probably worth maintaining the source directory option of cljs.closure/build for some time with a deprecation warning to reduce breakage.





[CLJS-575] cljsc.bat emit FileNotFoundException when compile samples in windows Created: 25/Aug/13  Updated: 05/Oct/13

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Park Sang Kyu Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: Compiler, bug, patch
Environment:

in windows 7


Attachments: File cljsc.bat.diff     File cljsc-path.bat    
Patch: Code

 Description   

cljsc.bat emit FileNotFoundException when it compile samples of the ClojureScript project in windows like below.

------------------------------------------------
Exception in thread "main" java.io.FileNotFoundException: Could not locate cljs/closure__init.class
or cljs/closure.clj on classpath:
------------------------------------------------

It is caused by lack of a backslash in the end of path of the system environment variable, %CLOJURESCRIPT_HOME% set by a user.
In the case CLASSPATH is set to "C:\\clojure\clojurescriptsrc\clj;C:\\clojure\clojurescriptsrc\cljs" and this make it impossible for javac to find cljs/clojure.clj file.

So it can be solved by adding a backslash to the path of %CLOJURESCRIPT_HOME%.

I attached the patched file, "cljsc-path.bat"



 Comments   
Comment by David Nolen [ 04/Sep/13 11:04 PM ]

Can we please get a proper git diff thanks (and please send in your CA)! Also would be nice to get Windows users to check this out.

Comment by Park Sang Kyu [ 15/Sep/13 3:16 AM ]

git diff

Comment by David Nolen [ 05/Oct/13 11:55 AM ]

Thank you! Have you sent in your CA? http://clojure.org/contributing





[CLJS-525] Allow hashtable lookup used for numbers and strings to be extended to other built-in types Created: 17/Jun/13  Updated: 17/Jun/13

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

...which would enable safe extension of key cljs protocols to types without modifying their prototypes, e.g. CLJS-523.



 Comments   
Comment by David Nolen [ 17/Jun/13 2:56 PM ]

Date is the only JS native case that I'm aware of that we don't handle. One tricky bit is that goog.typeOf won't give us the information we need, but I think instanceof should cover us here?

Comment by Fogus [ 17/Jun/13 3:05 PM ]

instanceof or the ever-gruesome toString.call(aDate) == '[object Date]' will work.





[CLJS-497] Constant literal optimization Created: 25/Apr/13  Updated: 27/Aug/13

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

We should optimize constant literals, in particular keywords. This optimization means that we will have to decide whether to make identical? slower by testing for keywords (this means it's probably a bad idea to continue to inline it) or to provide a special keyword-identical? that does the right thing.



 Comments   
Comment by Sean Grove [ 26/Aug/13 11:03 PM ]

This is related to the reified keywords in cljs, see http://dev.clojure.org/jira/browse/CLJS-576

Comment by Sean Grove [ 27/Aug/13 4:24 PM ]

There's another interesting twist while using piggieback + brepl that relates to a missing constants_table.js. Not sure what causes it (haven't found a way to repro), but only happens in a few circumstances, so the repl still mainly works.

The runtime part continues to works fine however.





[CLJS-374] satisfies? produces strange code when the protocol is not in the fast-path list Created: 06/Sep/12  Updated: 19/Nov/13

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: David Nolen Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None





[CLJS-365] apply needs to put all args after the 20th into an array seq Created: 29/Aug/12  Updated: 29/Aug/12

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

This ticket is related to CLJS-359






[CLJS-364] compiler needs to put all args of an invocation after 20 into an array-seq Created: 29/Aug/12  Updated: 29/Aug/12

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

This ticket is related to CLJS-359






[CLJS-109] Compiler errors/warnings should be displayed when cljs namespace 'package' names start with an unacceptable javascript symbol. Created: 29/Nov/11  Updated: 31/Aug/12

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Benjamin Conlan Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

OSX 10.7



 Description   

Clojurescript namespaces are extremely flexible. The generated javascript symbol names are not. ie:

cljs:
[code]
(ns canvas.context.2d)

(defn ^:export create-image-data
([canvas image-data] (.createImageData image-data))
([canvas sw sh] (.createImageData canvas sw sh)))
[/code]

generated js:
[code]
goog.provide('canvas.context.2d');
goog.require('cljs.core');
canvas.context.2d.create_image_data = (function() {
var create_image_data = null;
var create_image_data__2432 = (function (canvas,image_data){ return image_data.createImageData(); });
var create_image_data__2433 = (function (canvas,sw,sh){ return canvas.createImageData(sw,sh); });
create_image_data = function(canvas,sw,sh){
switch(arguments.length){ case 2 : return create_image_data__2432.call(this,canvas,sw); case 3 : return create_image_data__2433.call(this,canvas,sw,sh); }
throw('Invalid arity: ' + arguments.length);
};
return create_image_data;
})()
;
goog.exportSymbol('canvas.context.2d.create_image_data', canvas.context.2d.create_image_data);[/code]

Note the symbol name "canvas.context.2d.create_image_data". Because of the "2d" namespace name, the javascript generated is invalid.

This can simply be resolved by renaming the namespace but some notification to the developer during the compilation stage should help avoid unnecessary problems.



 Comments   
Comment by Jozef Wagner [ 09/Jan/12 2:59 PM ]

I had a similar problem when my namespace contained a word class, e.g. in namespace:

(ns foo.bar.class)

Advanced closure compiler produced an error treating class as a JS keyword.





[CLJS-27] Conditional compilation (or reading) Created: 22/Jul/11  Updated: 27/Jul/12

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Rich Hickey Assignee: Unassigned
Resolution: Unresolved Votes: 10
Labels: None

Attachments: File conditional-compilation-clojure.diff     File conditional-compilation-clojurescript.diff    
Patch: Code and Test
Approval: Vetted

 Description   

As people start trying to write libs that are portable between Clojure and ClojureScript they might need to have a bit of branching on target. N.B. supporting this means a change to Clojure, although it has general utility there as well.

Consider CL #+ #- reader macros - http://www.lispworks.com/documentation/lw50/CLHS/Body/02_dhq.htm



 Comments   
Comment by Roman Scherer [ 19/Jul/12 8:52 AM ]

The following patches include an implementation of Common Lisp's #+
and #- reader macros to allow conditional compilation/reading for
Clojure and ClojureScript.

The patches add a dynamic variable called features to the
clojure.core and cljs.core namespaces, that should contain the
supported features of the platform in question as keywords.

Unlike in Common Lisp, the variable is a Clojure set and not a list.
In Clojure the set contains at the moment the :clojure keyword, and in
ClojureScript the :clojurescript keyword.

I would like to get feedback on the names that are added to this
variable. Are those ok? Is :jvm for Clojure and :js for ClojureScript
better? Should ClojureScript add something like :rhino, :v8 or
:browser as well?

To run the ClojureScript tests, drop a JAR named "clojure.jar" that
has the Clojure patch applied into ClojureScript's lib directory.

Comment by David Nolen [ 19/Jul/12 12:18 PM ]

This is an enhancement so it probably requires a design page and extended discussion before it will go anywhere. Until that happens I'm marking this is as low priority.

Comment by Roman Scherer [ 19/Jul/12 1:45 PM ]

Ok. If someone could give me write access to the Clojure Dev Wiki I would be happy to start such a design page.

Comment by David Nolen [ 19/Jul/12 5:50 PM ]

If you've sent in your CA request permissions on the clojure-dev mailing list.

Comment by Roman Scherer [ 21/Jul/12 5:45 AM ]

I started a design page for this ticket in the Clojure Dev wiki:
http://dev.clojure.org/display/design/Feature+Expressions

Comment by Stuart Halloway [ 27/Jul/12 1:48 PM ]

Posting my comments over on the design page...





Generated at Sun Apr 20 13:53:08 CDT 2014 using JIRA 4.4#649-r158309.