<< Back to previous view

[CLJS-1067] Shared AOT cache for dependencies in JARs Created: 26/Feb/15  Updated: 22/Dec/17

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

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


 Description   

3rd party library code in JARs shouldn't be recompiled across dev and prod configurations. There should be a shared AOT cache for all builds within a project for all non-project local source.






[CLJS-1076] :nashorn target Created: 02/Mar/15  Updated: 27/Dec/17

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

Type: Enhancement Priority: Critical
Reporter: David Nolen Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: None


 Description   

To run well on Nashorn the target should supply CLOSURE_IMPORT_SCRIPT as well as setTimeout or setImmediate for core.async.






[CLJS-1501] Add :parallel-build support to REPLs Created: 05/Dec/15  Updated: 27/Jun/17

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

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


 Description   

The :parallel-build option does not currently work in REPLs due to the implementation of cljs.repl/load-namespace






[CLJS-968] Metadata on function literal inside of a let produces invalid Javascript Created: 07/Jan/15  Updated: 27/Jun/17

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

Type: Defect Priority: Major
Reporter: Bobby Eickhoff Assignee: David Nolen
Resolution: Unresolved Votes: 2
Labels: bug
Environment:

Originally found with [org.clojure/clojurescript "0.0-2496"]
Still reproducible with the latest cljsc (b5e9a5116259fc9f201bee4b9c6564f35306f9a5)



 Description   

Here is a minimal test case that produces the invalid Javascript:

(defn f []
  (let [a 0]
    ^{"meta" "data"}
    (fn [] true)))

The compiled Javascript includes the invalid token sequence "return return". (Per Chrome: Uncaught SyntaxError: Unexpected token return)

The problem does not occur if the metadata applies to a map literal instead of a function literal.
The problem only occurs when the function and metadata are inside of a let.



 Comments   
Comment by Bobby Eickhoff [ 07/Jan/15 9:45 PM ]

I forgot to try with-meta. Using with-meta does not produce this syntax error, so it's only a problem with the reader macro for metadata.

Comment by David Nolen [ 08/Jan/15 7:41 AM ]

Any quick thoughts about this one Nicola? Quite possibly a compiler issue on the CLJS side.

Comment by Nicola Mometto [ 08/Jan/15 8:07 AM ]

David, I understand why this happens but I don't know enough about how cljs's js emission to propose a fix.
The issue is that with this commit: https://github.com/clojure/clojurescript/commit/d54defd32d6c5ffcf6b0698072184fe8ccecc93a the following scenario is possible:

{:op :meta
 :env {:context :return}
 :expr {:op :fn
        :env {:context :expr}
        :methods [{:op :fn-method 
                   :env {:context :return} ..}]
        ..}
 ..}

i.e. analyze-wrap-meta changes the context of the :fn node to :expr but keeps the context of the :fn-methods to :return.

This causes both
https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/compiler.clj#L575-L576
and
https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/compiler.clj#L488 (https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/compiler.clj#L233)

to be true and emit a "return".

Comment by David Nolen [ 06/May/15 7:15 PM ]

Hrm, it appears analyze-wrap-meta may need to defer to a helper to change the :context of the given AST node.

Comment by Herwig Hochleitner [ 11/Dec/15 10:52 AM ]

I just randomly ran into this, when upgrading an old project. There is also a duplicate already: http://dev.clojure.org/jira/browse/CLJS-1482

Comment by Jonathan Chu [ 28/Jan/16 6:19 PM ]

This issue occurs for me even without a let.

(fn []
  ^{"meta" "data"}
  (fn [] true))

gives me

#object[SyntaxError SyntaxError: Unexpected token return]




[CLJS-719] this-as behaves incorrectly in "scoping function" Created: 07/Dec/13  Updated: 27/Jun/17

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

Type: Defect Priority: Major
Reporter: Kevin Marolt Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: None


 Description   

When a this-as expression gets put in a "scoping function", e.g. in a let-binding, the value bound via this-as refers to the scoping function, and not to the outer scope.

Example:

(def foo
  (js-obj
    "bar" "baz"
    "getBarRight" (fn [] (this-as self (.-bar self)))
    "getBarWrong" (fn []
                    (let [bar (this-as self (.-bar self))]
                      bar))))
     
(.log js/console (.getBarRight foo)) ;; => "baz"
(.log js/console (.getBarWrong foo)) ;; => undefined

Whereas foo.getBarRight expands to something like

function() {
  var self = this; // this refers to foo
  return self.bar; // returns "bar"
}

foo.getBarWrong on the other hand expands to

function() {
  var bar = function() {
    var self = this; // this refers to enclosing function
    return self.bar; // returns undefined
  }();
  return bar; // returns undefined
}





[CLJS-1896] Externs file validation Created: 23/Jan/17  Updated: 27/Jun/17

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

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


 Description   

Invalid externs file will fail silently and not provide the expected externs inference warnings. We should try to catch such issues when we parse the file and throw an exception.






[CLJS-2002] Don't throw when no *print-fn* is set Created: 07/Apr/17  Updated: 18/Aug/17

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

Type: Defect Priority: Major
Reporter: Thomas Heller Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: errors, warnings

Attachments: Text File CLJS-2002.patch    
Approval: Vetted

 Description   

Currently calling (enable-console-print!) causes a bunch of code to be retained in :advanced mode even if you never print.

While that is not ideal it doesn't cause runtime errors. Not calling it and trying to print however will throw an exception which will potentially break your app.

No *print-fn* fn set for evaluation environment

So we end up in a no-win situation for :advanced builds where a "forgotten" prn may break your app in production or "maybe" bloating your file size by retaining all the print-related things.

I think the no-print-fn condition should never throw, maybe just try to write a warning using console.log. Or just dropping the prn altogether.



 Comments   
Comment by David Nolen [ 13/Jul/17 8:29 PM ]

Let's move the old behavior to `string-print` only.

Comment by Mike Fikes [ 13/Jul/17 9:01 PM ]

The attached CLJS-2002.patch moves the throw to string-print and using nil as the init for the two Vars solves CLJS-2231. But it doesn't address the ticket as written: Leaving an inadvertent prn in your code leads to a call to string-print which throws.

Comment by David Nolen [ 13/Jul/17 10:06 PM ]

https://github.com/clojure/clojurescript/commit/797e247fbef676544060a57da995f058db061f37 partially addresses this issue. Keeping this open and moving to lower priority, we should revisit.





[CLJS-1702] Warning when using private vars Created: 07/Jul/16  Updated: 18/Aug/17

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

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

Approval: Vetted

 Description   

Currently no warning or error of any kind is given. Throwing an error and forcing users to go through vars is somewhat less attractive since vars dump information like file, line, etc. A warning would be a simple way to flag users that they are treading into dangerous territory. Downstream tooling error handling can make it a hard error if they like.






[CLJS-1701] cljs.spec impact on :advanced builds Created: 07/Jul/16  Updated: 07/Jul/16

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

Type: Defect Priority: Major
Reporter: Thomas Heller Assignee: David Nolen
Resolution: Unresolved Votes: 3
Labels: None


 Description   

Investigate the impact of cljs.spec on :advanced builds.

Currently all specs are kept in the (private) cljs.spec/registry-ref atom. This atom is not understood by the Closure Compiler and cannot be eliminated as dead code. So even if specs are not used in "production" they still bloat the generated JS size. Some specs may be used at runtime and cannot not be removed, the gen parts however are probably never required in :advanced builds and should be omitted somehow.

In a test build (with 1.9.93) this adds 11kb (102kb vs 91kb) as soon as cljs.spec is :require'd somewhere and goes up with each defined spec.






[CLJS-1300] REPLs do no write out updated deps.js when compiling files Created: 05/Jun/15  Updated: 27/Jun/17

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

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

Attachments: Text File cljs-1300.patch    
Patch: Code

 Description   

For example a user may edit a file including a new dependency. This will work at the REPL but if a browser refresh is made the emitted goog.require will fail due to the initial deps.js file being stale.



 Comments   
Comment by ewen grosjean [ 05/Dec/15 4:15 PM ]

load-file is broken into 4 sub-functions:
repl-compile-cljs: compile the cljs file beeing loaded
repl-cljs-on-disk: ensures all dependencies are on disk
refresh-cljs-deps: refreshes the cljs_deps.js file
repl-eval-compiled: eval the compiled file

Comment by David Nolen [ 05/Dec/15 9:02 PM ]

Thanks will review.

Comment by Mike Fikes [ 31/Jan/16 3:25 PM ]

cljs-1300.patch no longer applies on master





[CLJS-868] no arity warnings on recursive calls Created: 03/Oct/14  Updated: 19/Nov/17

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

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

Attachments: File cljs_868_14_Nov_2015.md     Text File cljs_868_14_Nov_2015.patch    
Patch: Code

 Description   

If a function recursively invokes itself within its own body the invoke will not be checked for arity mismatch.



 Comments   
Comment by Samuel Miller [ 10/Aug/15 10:06 PM ]

Took some time to look at this issue. Originally thought "Do what loop/recur does" but that does not take into account multi-arity. It seems like maybe the best option is to somehow use the second pass of the analyze(analyze-fn-methods-pass2). The entire information about the function is present and the warning section of the code gets triggered but because of no-warn is ignored. Any other ideas for a solution to this?

Comment by Samuel Miller [ 14/Nov/15 7:47 PM ]

So I am looking for feed back on this patch and I will try to explain the reasoning for each section.

The issue is that a function only knows about it's arity after it has been parsed once.
So we need to check arity issues on the second pass

First off, added two new variables.
-activate-second-pass-warnings:Boolean Basically if you want to have second-pass warnings turned on
-second-pass-cljs-warnings:Set Right now we only have :fn-arity but I figure might as well make it generic.

So first up if the modifications to the analyze-fn-methods-pass2 function.
Instead of using no-warn marco here we have some new functionality.
The goal is to turn everything off except the second-pass warnings

So if activate-second-pass-warnings is false just use no-warn else it will use the new section of code.

The default-warning-handler was also modified. After checking if a warning is on, it checks if the warning is a second-pass warning and
if that warning can now be activated. If activate-second-pass-warnings is false AND a warning is still on that implies it is a second pass warning
in the second pass so we activate it.

Also I tried to keep all modifications in cljs.analyzer.

Originally I had the cljs-warnings :fn-arity to false and it would only be turned on in the second pass.
However the repl section just sets everything to true (and turns off select parts like ns errors).
So I decided to not touch those sections and instead keep how other files interface with the analyzer the same.

Comment by Samuel Miller [ 16/Nov/15 10:58 PM ]

Just realized that I have the patch marked as .md instead of .patch

Comment by Mike Fikes [ 19/Nov/17 8:09 PM ]

Patch no longer applies.





[CLJS-1558] Code allowed to re-define referred var Created: 31/Jan/16  Updated: 27/Nov/17

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

Type: Defect Priority: Major
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File CLJS-1558.patch    
Patch: Code and Test

 Description   

If you refer a var from another namespace, then you can def a new value for that var, and the def will mutate the other namespace, and other things will go wrong as illustrated in the example below.

FWIW, Clojure disallows this, and refuses to allow you to evaluate a def involving a referred var, and emits an error diagnostic like:

CompilerException java.lang.IllegalStateException: foo already refers to: #'some.name.space/foo in namespace: user, compiling:(NO_SOURCE_PATH:2:1)

Here is a complete example illustrating the issues:

Given:

(ns foo.core)

(defn square [x]
  (* x x))

then do this in a REPL:

cljs.user=> (require '[foo.core :refer [square]])
nil
cljs.user=> (var square)
#'foo.core/square
cljs.user=> (square 3)
9
cljs.user=> (ns-interns 'cljs.user)
{}
cljs.user=> (defn square [x] (+ x x))
WARNING: square already refers to: foo.core/square being replaced by: cljs.user/square at line 1 <cljs repl>
#'foo.core/square
cljs.user=> (square 3)
6
cljs.user=> (var square)
#'foo.core/square
cljs.user=> (in-ns 'foo.core)
nil
foo.core=> (square 3)
6
foo.core=> (in-ns 'cljs.user)
nil
cljs.user=> (ns-interns 'cljs.user)
{square #'cljs.user/square}
cljs.user=> (cljs.user/square 3)
TypeError: Cannot read property 'call' of undefined
    at repl:1:105
    at repl:9:3
    at repl:14:4
    at Object.exports.runInThisContext (vm.js:54:17)
    at Domain.<anonymous> ([stdin]:41:34)
    at Domain.run (domain.js:221:14)
    at Socket.<anonymous> ([stdin]:40:25)
    at emitOne (events.js:77:13)
    at Socket.emit (events.js:169:7)
    at readableAddChunk (_stream_readable.js:146:16)
cljs.user=> #'cljs.user/square
#'cljs.user/square
cljs.user=> @#'cljs.user/square
nil


 Comments   
Comment by Mike Fikes [ 27/Nov/17 2:00 PM ]

The attached patch essentially turns the warning into an error for non-core names, throwing an exception that matches Clojure's.

Example:

$ script/noderepljs
ClojureScript Node.js REPL server listening on 49203
To quit, type: :cljs/quit
cljs.user=> (def map 3)
WARNING: map already refers to: cljs.core/map being replaced by: cljs.user/map at line 1 <cljs repl>
#'cljs.user/map
cljs.user=> (require '[clojure.set :refer [intersection]])
nil
cljs.user=> (def intersection 3)
clojure.lang.ExceptionInfo: intersection already refers to: #'clojure.set/intersection in namespace cljs.user at line 1 <cljs repl> {:file "<cljs repl>", :line 1, :column 1, :root-source-info {:source-type :fragment, :source-form (def intersection 3)}, :tag :cljs/analysis-error}
	at clojure.core$ex_info.invokeStatic(core.clj:4725)
	at clojure.core$ex_info.invoke(core.clj:4725)
	at cljs.analyzer$error.invokeStatic(analyzer.cljc:697)
...
Comment by Mike Fikes [ 27/Nov/17 2:05 PM ]

Here is an example showing that it is (as was before), OK to define a Var that shadows a core Var:

$ script/noderepljs
ClojureScript Node.js REPL server listening on 57077
To quit, type: :cljs/quit
cljs.user=> (defn quot [s] (str "'" s "'"))
WARNING: quot already refers to: cljs.core/quot being replaced by: cljs.user/quot at line 1 <cljs repl>
#'cljs.user/quot
cljs.user=> (quot "hello")
"'hello'"
cljs.user=> (cljs.core/quot 6 2)
3




[CLJS-2247] Warn when overwriting protocol method Created: 15/Jul/17  Updated: 29/Dec/17

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

Type: Enhancement Priority: Major
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: errormsgs

Attachments: Text File CLJS-2247-2.patch     Text File CLJS-2247.patch    
Patch: Code and Test

 Description   

Warn when a protocol overwrites a method in another protocol.

Observe the warning in this Clojure REPL session:

user=> (defprotocol IAlpha
  #_=>   (-foo [this]))
IAlpha
user=> (defrecord Alpha []
  #_=>   IAlpha
  #_=>  (-foo [this] :alpha))
user.Alpha
user=> (defprotocol IBeta
  #_=>   (-foo [this]))
Warning: protocol #'user/IBeta is overwriting method -foo of protocol IAlpha
IBeta
user=> (defrecord Beta []
  #_=>   IBeta
  #_=>   (-foo [this] :beta))
user.Beta
user=> (-foo (->Alpha))

IllegalArgumentException No implementation of method: :-foo of protocol: #'user/IBeta found for class: user.Alpha  clojure.core/-cache-protocol-fn (core_deftype.clj:568)

Here is the same in ClojureScript:

To quit, type: :cljs/quit
cljs.user=> (defprotocol IAlpha
(-foo [this]))
nil
cljs.user=> (defrecord Alpha []
IAlpha
(-foo [this] :alpha))
cljs.user/Alpha
cljs.user=> (defprotocol IBeta
(-foo [this]))
nil
cljs.user=> (defrecord Beta []
IBeta
(-foo [this] :beta))
cljs.user/Beta
cljs.user=> (-foo (->Alpha))
repl:13
throw e__5612__auto__;
^

Error: No protocol method IBeta.-foo defined for type : [object Object]
    at cljs$core$missing_protocol (/Users/mfikes/.cljs_node_repl/.cljs_node_repl/cljs/core.js:317:9)
    at cljs$user$_foo (repl:25:34)
    at repl:1:94
    at repl:9:3
    at repl:14:4
    at ContextifyScript.Script.runInThisContext (vm.js:44:33)
    at Object.runInThisContext (vm.js:116:38)
    at Domain.<anonymous> ([stdin]:50:34)
    at Domain.run (domain.js:242:14)
    at Socket.<anonymous> ([stdin]:49:25)

Note that they both result in the same runtime behavior, but Clojure emits a nice diagnostic when IBeta is defined.



 Comments   
Comment by Mike Fikes [ 16/Jul/17 9:28 AM ]

This is an interesting one: While the attached patch produces nice warnings, even without it the :redef-in-file warning triggers if the forms involved happen to reside in files, and in that case you'd get two slightly different warnings being emitted:

WARNING: Protocol IDelta is overwriting method -bar of protocol IGamma in file /Users/mfikes/Desktop/src/foo/core.cljc
WARNING: -bar at line 6 is being replaced at line 7 /Users/mfikes/Desktop/src/foo/core.cljc

Each of the warnings above has their own strengths; but having both seems to be a negative. Here are my initial thoughts on variants:

  1. Leave both: Could be perceived to be noisy, albeit useful if it occurs.
  2. Have the warning disabled, but enabled for the REPL: Odd to have a warning that only happens while in the REPL (and you can still get two when loading files)
  3. Have the warning only be emitted for REPL-entered forms: This perhaps directly addresses the issue: (No double warnings, useful for people learning the language). But this seems even odder to have a warning limited to REPL-entered forms.
  4. Figure out a way to suppress the :redef-in-file warning in this case: Maybe cleaner.
  5. Same as above, but find a way to get the line numbers into the "overwrite" warning: Perhaps cleanest, but difficult to do?
  6. Do nothing, decline the ticket: Perhaps this leaves users learning the language at a loss if they encounter this while in the REPL where redefining forms is "normal", but not overwriting via protocol definitions.
Comment by Mike Fikes [ 29/Dec/17 10:19 AM ]

The attached CLJS-2247-2.patch rebaselines against master.





[CLJS-2460] Print MapEntry as vector Created: 08/Jan/18  Updated: 08/Jan/18

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

Type: Enhancement Priority: Major
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: print, printing

Attachments: Text File CLJS-2460.patch    
Patch: Code and Test

 Description   

(pr-str (->MapEntry :a 1 nil)) yields {{"#object[cljs.core.MapEntry]"}} instead of "[:a 1]".



 Comments   
Comment by Mike Fikes [ 08/Jan/18 8:25 AM ]

The attached patch also tests that printing matches Clojure in the case that *print-length* happens to be less than 2.





[CLJS-2456] Have map types return MapEntry instead of vector Created: 03/Jan/18  Updated: 08/Jan/18

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

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

Attachments: Text File CLJS-2456.patch    
Patch: Code and Test

 Description   

Groundwork for this has been done.

CLJS-2013 adds the MapEntry type, but indicates "PAMs and PHMs don't emit them when you seq or iterate over them yet."
CLJS-2068 fixes an issue with MapEntry
CLJS-2001 adds a map-entry? predicate

So this ticket entails updating the PHM and PAM implementations so that, for example, (map-entry? (first {:a 1})) evaluates to true.

Note: One additional bit of groundwork that should be done prior to this ticket is CLJS-2460.



 Comments   
Comment by Thomas Mulvaney [ 06/Jan/18 6:46 PM ]

I've attached a patch for consideration. Besides updating `Seq` and `Iterator` types for PAM and PHM, it also updates implementers of `IFind` to return `MapEntry` rather than vectors. I can move the `IFind` changes into another patch though if that is prefered.





[CLJS-2462] subvec on non-integral indexes fails Created: 09/Jan/18  Updated: 09/Jan/18

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

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

Attachments: Text File CLJS-2462.patch    
Patch: Code and Test

 Description   

In Clojure, you can evidently pass non-integral indexes to count. For example (count (subvec [1 2] 0 1.5)) evaluates to 1 in Clojure. But, in ClojureScript, this evaluates to 1.5.

It is not clear whether passing non-integral indexes is legal, or if doing so leads to undefined behavior (in which case this ticket is invalid).



 Comments   
Comment by Mike Fikes [ 09/Jan/18 11:30 AM ]

We had discussed this in #cljs-dev, and concluded that subvec should round its arguments down to the nearest integer. Theoretically speaking, Math/floor is the correct operation, given that vector indexes can go up to 2^32 - 1, but pragmatically speaking, int is a much more compelling choice because it is faster (compiling down to a bit-or with zero). This choice would cause errors if the arguments happen to be greater than 2147483647.





[CLJS-2389] Fix module processing after latest Closure changes Created: 27/Oct/17  Updated: 12/Jan/18

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

Type: Defect Priority: Major
Reporter: Juho Teperi Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File CLJS-2389-1.patch     Text File CLJS-2389-3.patch     Text File CLJS-2389-4.patch     Text File CLJS-2389-5.patch     Text File CLJS-2389-wip-2.patch     Text File CLJS-2389-wip.patch    

 Description   

New Closure-compiler doesn't add goog.provide/require calls to processed modules:

https://github.com/google/closure-compiler/pull/2641

> ES6 and CommonJS modules no longer generate synthetic goog.require and goog.provide calls.

Currently {process-js-modules} uses {load-library}, which reads the goog.provide calls in the file, to determine the name for processed module, something like {module$absolute$path}.
Now that the file doesn't have goog.provide call, this breaks.

As the module name is based on file-path, we can determine the module name directly from filepath, Closure provides utility for this: {ModuleNames/fileToModuleName}.



 Comments   
Comment by Juho Teperi [ 27/Oct/17 5:53 AM ]

Attached patch fixes the first problem, which is updating {:js-module-index} mapping from Node name to processed module name.

Another problem is that analyzer/check-uses now fails when referring to symbols in processed modules:

ERROR in (test-module-name-substitution) (core.clj:4617)
expected: (= (compile (quote (ns my-calculator.core (:require [calculator :as calc :refer [subtract add] :rename {subtract sub}])))) (str "goog.provide('my_calculator.core');" crlf "goog.require('cljs.core');" crlf "goog.require('" (absolute-module-path "
src/test/cljs/calculator.js" true) "');" crlf))
  actual: clojure.lang.ExceptionInfo: Invalid :refer, var module$home$juho$Source$clojurescript$src$test$cljs$calculator/add does not exist

For some reason {missing-use?} doesn't work correctly.

Comment by Juho Teperi [ 06/Dec/17 12:47 PM ]

I'm looking into emitting those goof.provide/require calls from Cljs. I have provide working already and that fixes analyzer etc. But we also need require so that cljs_deps.js gets proper information for browser, but I haven't yet found a way to retrieve module requires from Closure.

Comment by Juho Teperi [ 12/Dec/17 12:20 PM ]

New patch should now emit goog.provide/require calls and tests pass, and I have tested this with Reagent and npm modules and foreign libs. Works with Closure v20171112 but v20171203 doesn't seem to process CommonJS at all, but doesn't give any errors.

One problem is that optmized build now prints bunch of these warnings:

Dec 12, 2017 8:10:28 PM com.google.javascript.jscomp.PhaseOptimizer$NamedPass process
WARNING: Skipping pass inlineTypeAliases
Dec 12, 2017 8:10:29 PM com.google.javascript.jscomp.PhaseOptimizer$NamedPass process
WARNING: Skipping pass j2clChecksPass

Reagent advanced test build size increased from 1.1M to 1.3M so it is possible (some) optimizations are not being applied.

Comment by Thomas Heller [ 12/Dec/17 12:53 PM ]

I think those warnings only appear when `:language-in` is NOT set. Setting it to `:ecmascript6` now does weird things to CLJS though which may explain your code size increase.

Comment by Juho Teperi [ 12/Dec/17 12:55 PM ]

Hmm, I looked into CompilerOptions and I thought it defaults to EcmaScript2017 (ES8_MODULES featureSet, which should enable everything), but probably I missed something.

Comment by Thomas Heller [ 12/Dec/17 4:56 PM ]

Could be that the default is too high. The warnings went away (in shadow-cljs) when setting :language-in to :ecmascript5 and I didn't look any further since that is fine for CLJS.

Comment by Juho Teperi [ 08/Jan/18 5:18 PM ]

Current patch status:

Latest v20180101 release processes CJS and ES6 OK.

The emitted JS code doesn't work, because all methods exported from CJS (and ES6 default exports) need to be accessed through default property, like:

module$path$node_modules$left_pad$index["default"](...)

module$path$node_modules$react$react["default"].cloneElement(...)

And still seeing warnings about skipping passes.

Comment by Juho Teperi [ 09/Jan/18 4:06 PM ]

Latest version modifies compiler to emit ["default"] when accessing anything in JS modules.

Still getting warnings during optimization. Setting language-in doesn't help.

Comment by Juho Teperi [ 09/Jan/18 4:34 PM ]

-4 now defaults language-in to EcmaScript 5 so all optimization passes get enabled. Optimized output size now matches 1.9.946 when tested with Reagent.

Comment by Juho Teperi [ 10/Jan/18 2:22 PM ]

-5 reads module type from Closure, adds it to new :js-namespaces compiler env property, and uses that on emit* :var to emit ["default"] for only CJS modules.

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

I'm unable to get this patch working with `script/test`. Module processing fails on lodash package.json because we are parsing as JS source. I also modified `script/test` to invoke the new `clj` tool and leverage the new `deps.edn` file so we can use the unshaded Closure Compiler dependency and I observed no change - the same issue persists.





[CLJS-1278] Asserts still fail while :require-ing .js file (either in :libs or in :source-paths) (same as CLJS-1196) Created: 20/May/15  Updated: 14/Jul/15

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

Type: Defect Priority: Minor
Reporter: Michal Till Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File cljs_1278.patch    

 Description   

Following on CLJS-1196, I can't get it to work.

In version 0.0-3264 lein-cljsbuild crashed on weird eception `Caused by: java.lang.IllegalArgumentException: No implementation of method: :make-reader of protocol: #'clojure.java.io/IOFactory found for class: nil"` but the current version 0.0-3269 gives the same failed assertion as previously.

I've put up a sample project to illustrate the issue.

Steps to reproduce:

`git clone https://github.com/tillda/stackone`
`cd stackone`
`git checkout 537e5c69b844bc53c159e85cafc24310543cc918`
`lein clean && lein cljsbuild once temp`

Expected behaviour: cljs compiled successfully with src/vendor/client/closure.js and env/stackone/helpersjs.js being included.

Actual behaviour:

```
Compiling "resources/public/lein-cljsbuild-temp/dev-mode-deps.js" failed.
Exception in thread "main" java.lang.AssertionError: Assert failed: (or (file? x) (url? x) (string? x)), compiling/private/var/folders/ym/l2qxd7l97kzfzftrdpqsclm40000gn/T/form-init3642888309490821030.clj:1:125)
at clojure.lang.Compiler.load(Compiler.java:7249)
at clojure.lang.Compiler.loadFile(Compiler.java:7175)
at clojure.main$load_script.invoke(main.clj:275)
at clojure.main$init_opt.invoke(main.clj:280)
at clojure.main$initialize.invoke(main.clj:308)
at clojure.main$null_opt.invoke(main.clj:343)
at clojure.main$main.doInvoke(main.clj:421)
at clojure.lang.RestFn.invoke(RestFn.java:421)
at clojure.lang.Var.invoke(Var.java:383)
at clojure.lang.AFn.applyToHelper(AFn.java:156)
at clojure.lang.Var.applyTo(Var.java:700)
at clojure.main.main(main.java:37)
Caused by: java.lang.AssertionError: Assert failed: (or (file? x) (url? x) (string? x))
at cljs.util$ext.invoke(util.cljc:115)
at cljs.closure$source_on_disk.invoke(closure.clj:1206)
at cljs.closure$output_unoptimized$fn__3708.invoke(closure.clj:1235)
at clojure.core$map$fn__4551.invoke(core.clj:2622)
at clojure.lang.LazySeq.sval(LazySeq.java:40)
at clojure.lang.LazySeq.seq(LazySeq.java:49)
at clojure.lang.RT.seq(RT.java:507)
at clojure.core$seq__4126.invoke(core.clj:135)
at clojure.core$filter$fn__4578.invoke(core.clj:2677)
at clojure.lang.LazySeq.sval(LazySeq.java:40)
at clojure.lang.LazySeq.seq(LazySeq.java:49)
at clojure.lang.RT.seq(RT.java:507)
at clojure.core$seq__4126.invoke(core.clj:135)
at clojure.core$map$fn__4551.invoke(core.clj:2614)
at clojure.lang.LazySeq.sval(LazySeq.java:40)
at clojure.lang.LazySeq.seq(LazySeq.java:49)
at clojure.lang.Cons.next(Cons.java:39)
at clojure.lang.RT.next(RT.java:674)
at clojure.core$next__4110.invoke(core.clj:64)
at clojure.core$str$fn__4186.invoke(core.clj:528)
at clojure.core$str.doInvoke(core.clj:526)
at clojure.lang.RestFn.applyTo(RestFn.java:139)
at clojure.core$apply.invoke(core.clj:628)
at cljs.closure$deps_file.invoke(closure.clj:1040)
at cljs.closure$output_deps_file.invoke(closure.clj:1060)
at cljs.closure$output_unoptimized.doInvoke(closure.clj:1243)
at clojure.lang.RestFn.applyTo(RestFn.java:139)
at clojure.core$apply.invoke(core.clj:630)
at cljs.closure$build.invoke(closure.clj:1514)
at cljs.closure$build.invoke(closure.clj:1426)
at cljsbuild.compiler$compile_cljs$fn__3884.invoke(compiler.clj:81)
at cljsbuild.compiler$compile_cljs.invoke(compiler.clj:80)
at cljsbuild.compiler$run_compiler.invoke(compiler.clj:187)
at user$eval4018$iter_40544058$fn4059$fn_4077.invoke(form-init3642888309490821030.clj:1)
at user$eval4018$iter_40544058$fn_4059.invoke(form-init3642888309490821030.clj:1)
at clojure.lang.LazySeq.sval(LazySeq.java:40)
at clojure.lang.LazySeq.seq(LazySeq.java:49)
at clojure.lang.RT.seq(RT.java:507)
at clojure.core$seq__4126.invoke(core.clj:135)
at clojure.core$dorun.invoke(core.clj:3007)
at clojure.core$doall.invoke(core.clj:3023)
at user$eval4018.invoke(form-init3642888309490821030.clj:1)
at clojure.lang.Compiler.eval(Compiler.java:6792)
at clojure.lang.Compiler.eval(Compiler.java:6782)
at clojure.lang.Compiler.load(Compiler.java:7237)
... 11 more
Subprocess failed
```



 Comments   
Comment by David Nolen [ 20/May/15 10:21 AM ]

This issue is in danger of being closed. Please supply minimal steps to reproduce that do not involve anything other than the ClojureScript compiler. We no longer have time to wade through the indirection introduced by cljsbuild or any other downstream tooling. Thanks.

Comment by Michal Till [ 20/May/15 11:14 AM ]

@David Nolen: I have created a failing minimal testcase based on the Quick Start document. Here it is: https://github.com/tillda/cljs-testcase/

Comment by David Nolen [ 20/May/15 11:27 AM ]

Michal the failing example is not correct. You are not supplying any :libs option.

Comment by Michal Till [ 20/May/15 11:45 AM ]

Ah! Thank you very much! This additional issue was therefore my error. Now it seems to work even in my "big" example.

However it would be cool if there was a meaningful error message stating that a file path can't be resolved. If one is not an expert in the cljs compiler this is almost impossible to figure out. After all the error message in the CLJS-1196 issue and in this wrongfully reported one are exactly the same.

You may close this issue.

Comment by David Nolen [ 20/May/15 11:55 AM ]

We'll leave it open for the improving the error message.

Comment by Sebastian Bensusan [ 22/May/15 7:16 AM ]

Added the check in cljs.closure/source-on-disk where there is info for the error message.

For the supplied case, the error message is:

java.lang.IllegalArgumentException: The file file:/home/carlos/Playground/cljs-testcase/src/hello_world/closure.js 
lacks an associated source file. If it is a JavaScript library please add it to :libs}}

If a different wording or location of the check is needed, I'll submit a new patch with corrections.

Notes:

  • Changed:(:provides js) to (-provides js) in order to be consistent with IJavaScript.
  • cljs.clojure/source-on-disk takes a js argument that should satisfy with IJavaScript and ISourceMap if :source-map is enabled but the implementation is hardcoded to maps because :source-map and :source-url are used instead of ISourceMap methods -source-map and -source-url. I propose to extend PersistentMap and PersistentArrayMap to ISourceMap to make source-on-disk compliant with both protocols.




[CLJS-404] Automate Browser REPL testing Created: 23/Oct/12  Updated: 05/Jan/16

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

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


 Description   

It's worth investigating Selenium, PhantomJS, etc. as solutions to sanity check the Browser REPL when we run the other tests.



 Comments   
Comment by Robert Krahn [ 22/Dec/14 1:22 PM ]

An attempt: https://github.com/clojure/clojurescript/pull/42

Comment by David Nolen [ 24/Dec/14 8:57 AM ]

This looks like an interesting patch, thanks!

Comment by Robert Krahn [ 26/Dec/14 10:57 AM ]

I'll post a patch here, first I'll investigate the load-file issue, though.





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

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

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





[CLJS-1494] turn cljs.core/*assert* into a goog-define Created: 25/Nov/15  Updated: 22/Feb/16

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

Type: Enhancement Priority: Minor
Reporter: Thomas Heller Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File goog-define-assert.patch    
Patch: Code

 Description   

This patch turns the cljs.core/*assert* boolean into a goog.define and also checks *assert* at runtime (instead of only at compile-time).

The closure define option allows the closure compiler to eliminate asserts in :advanced, while :none builds can keep the asserts. This is one of the few remaining issues that prevent :advanced builds to re-use :none compiled (cached) files.

:elide-asserts is unaffected to keep this as simple as possible, but could be built on top of the goog.define instead of actually affecting the compiled output.



 Comments   
Comment by Mike Fikes [ 20/Feb/16 8:02 AM ]

Patch no longer applies, probably owing to CLJS-970.

Comment by Thomas Heller [ 22/Feb/16 5:08 AM ]

There was one more issue I discovered with my approach. My goal was to enable the Closure Compiler to eliminate the asserts when using :advanced compilation. This works perfectly fine with using a goog.define for *assert* but the compiler will complain if you try to adjust the define later since goog.define vars are not allowed to be adjusted at runtime.

(binding [*assert* false]
  (something-that-asserts))

This works in CLJ but not in CLJS since *assert* is only checked at compile time. If compiled with :elide-asserts true you can't bind assert to true either since the code no longer exists.

So some compromise must be made either way, the best solution IMHO would be to have a goog.define which lets the compiler decide whether to eliminate the asserts or not, independent from the *assert* and then moving the assert check itself into js instead of the compiler.

Happy to write the patch if interested.





[CLJS-1924] The compiler cannot infer the target type of "(. RecordName -prototype)" expressions Created: 01/Feb/17  Updated: 01/Feb/17

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

Type: Defect Priority: Minor
Reporter: Viktor Magyari Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: compiler, extern


 Description   

Repro:

Place

(set! *warn-on-infer* true)

(defrecord Foo [])

anywhere in your source files, compile with :infern-externs true.

Expected:

Multiple warnings like:

  • WARNING: Cannot infer target type in expression (. Foo -prototype)
  • WARNING: Cannot infer target type in expression (. other__8838__auto__ -constructor)
  • WARNING: Cannot infer target type in expression (. user/Foo -getBasis)

There are also warnings for (. cljs.core/List -EMPTY), but this might be unrelated.






[CLJS-1970] Cannot infer target type for list/vector expressions Created: 08/Mar/17  Updated: 10/Mar/17

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

Type: Defect Priority: Minor
Reporter: Daniel Janus Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Ubuntu 16.04.2, Oracle JRE 8



 Description   

With

(set! *warn-on-infer* true)
enabled, attempting to compile functions like:

(defn foo [] (list))
(defn bar [] (vector))

results in a warning:

WARNING: Cannot infer target type in expression (. cljs.core/List -EMPTY) at line 2427 src/cljs/discann/core.cljs

WARNING: Cannot infer target type in expression (. cljs.core/PersistentVector -EMPTY) at line 2435 src/cljs/discann/core.cljs

The line number reported is totally unrelated to the line of code where the problematic fn appears.

Affects 1.9.456 and 1.9.494.






[CLJS-1164] quot and rem are inefficient Created: 24/Mar/15  Updated: 25/May/17

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

Type: Enhancement Priority: Minor
Reporter: Francis Avila Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: math

Attachments: Text File CLJS-1164-1.patch     Text File cljs-1164.patch    
Patch: Code and Test

 Description   

The implementation of the quot and rem functions are needlessly complicated. Currently they are:

(defn quot [n d] (fix (/ (- n (js-mod n d)) d)))
(defn rem [n d] (- n (* d (quot n d))))

However all numbers in js are doubles already, so all this is unnecessary:

(defn quot [n d] (fix (/ n d)))
(defn rem [n d] (js-mod n d)))

Notice that "rem" is simply js-mod, and I'm not sure why no one noticed this before. I keep js-mod for now since a lot of code uses it, and if cljs ever grows a number tower the distinction may be important.

Patch attached, which also:

  • Creates a macro version of quot and rem.
  • Updates documentation for quot, rem, js-mod and mod for clarity.
  • Implement fix (private function to round to zero) with ES6 Math.trunc() if available.

Existing quot and rem tests pass, although there could be some better tests of edge cases (negative decimal num or div, NaN and +-Infinity args).



 Comments   
Comment by Francis Avila [ 24/Mar/15 12:27 PM ]

Better tests found rounding errors in my updated rem, which should stay as-is. (Not simply js-mod after all! Seems to round args first? Not obvious from the spec.) Changed quot however is correct and introduces less error than the current one. Will update patch and tests when I get a chance.

Comment by Francis Avila [ 29/Mar/15 12:39 AM ]

Working patch with tests attached. Tests expanded to cover floating-point cases. rem is now fundamentally the same as master (was more accurate than js-mod!!), but returns results consistent with js-mod for non-finite args or zero divisor.

Comment by Mike Fikes [ 31/Jan/16 3:23 PM ]

cljs-1164.patch no longer applies on master

Comment by Andrea Richiardi [ 14/Feb/16 9:02 PM ]

Patch now applies. I only tested with Nashorn:

V8_HOME not set, skipping V8 tests
SPIDERMONKEY_HOME not set, skipping SpiderMonkey tests
JSC_HOME not set, skipping JavaScriptCore tests
Testing with Nashorn

...

Ran 185 tests containing 17195 assertions.
0 failures, 0 errors.
Tested with 1 out of 4 possible js targets
Comment by Andrea Richiardi [ 14/Feb/16 9:02 PM ]

Patch cleaned up

Comment by Mike Fikes [ 20/Feb/16 10:11 PM ]

Successfully ran Andrea's update to Francis's patch through V8, SpiderMonkey, JavaScriptCore, and Nashorn unit tests.

I also manually ran some of the unit tests in bootstrapped ClojureScript built with the patch.

LGTM.

Comment by Mike Fikes [ 20/Feb/16 10:23 PM ]

Since this is a low-level numerics update, also ran the unit tests through ChackraCore (successfully).

Comment by Mike Fikes [ 25/May/17 7:29 PM ]

CLJS-1164-1.patch no longer applies to master





[CLJS-2103]  clarify arg type and order constraints of keys and vals Created: 19/Jun/17  Updated: 19/Jun/17

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File CLJS-2103.patch    
Patch: Code

 Description   

Backport CLJ-1302 to ClojureScript, while also making the argument name be map instead of hash-map.






[CLJS-1415] Handling JSDoc param name [x] optional syntax Created: 10/Aug/15  Updated: 27/Jun/17

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

Type: Enhancement Priority: Minor
Reporter: David Nolen Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: type-check





[CLJS-2147] apply test suit Created: 01/Jul/17  Updated: 01/Jul/17

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

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: test

Attachments: Text File CLJS-2147.patch    
Patch: Code and Test

 Description   

Created an apply test suit.

Few tests are commented out which currently fail (nothing new and there are tickets for them).



 Comments   
Comment by David Nolen [ 01/Jul/17 1:56 PM ]

Please remove #_ following each test.

Comment by A. R [ 01/Jul/17 2:14 PM ]

Forgot about those. Done.





[CLJS-2127] Add invoke* helper macro Created: 26/Jun/17  Updated: 03/Jul/17

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

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: refactoring

Attachments: Text File CLJS-2127.patch    
Patch: Code
Approval: Screened

 Description   

This is a simple refactor around {IFn} protocol around core.cljc and core.cljs. We would like to hide the details of the invocation naming convention to avoid simple errors as well as to support changes more simply.



 Comments   
Comment by David Nolen [ 29/Jun/17 7:05 AM ]

The scope of this ticket needs to be narrowed to make it simpler for me to review. For the time being the only thing I would like to see is `invoke*` which hides the naming convention for direct invokes. No other higher level macro helpers should be provided in the resolution of this sissue.

Comment by A. R [ 29/Jun/17 12:29 PM ]

Patch updated. Much fewer changes to keep it simple for now.

Comment by David Nolen [ 29/Jun/17 2:08 PM ]

Looking better but lets have one helper for constructing the name, should just take number or :variadic.





[CLJS-2196] SpiderMonkey path needs quoting in test scripts Created: 08/Jul/17  Updated: 08/Jul/17

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

Type: Defect Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-2196.patch    
Patch: Code

 Description   

The paths to the other engines (V8, Nashorn, etc.), are quoted, allowing paths with spaces. This needs to also be done for SpiderMonkey.






[CLJS-2301] Avoid use of deprecated goog.string/isEmptySafe in clojure.string/blank? Created: 05/Aug/17  Updated: 05/Aug/17

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-2301.patch    
Patch: Code

 Description   

clojure.string/blank? calls goog.string/isEmptySafe, which is marked as deprecated. Instead this can be inlined with the code that this internally calls, which is non-deprecated code. Also, it can be seen that such a change has no effect on perf, with these benchmarking tests tried out:

Before:

Benchmarking with V8
[s nil f clojure.string/blank?], (f s), 1000000 runs, 27 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 28 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 26 msecs
Benchmarking with SpiderMonkey
[s nil f clojure.string/blank?], (f s), 1000000 runs, 265 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 268 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 268 msecs
Benchmarking with JavaScriptCore
[s nil f clojure.string/blank?], (f s), 1000000 runs, 4 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 4 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 5 msecs
Benchmarking with Nashorn
[s nil f clojure.string/blank?], (f s), 1000000 runs, 331 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 336 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 59 msecs
Benchmarking with ChakraCore
[s nil f clojure.string/blank?], (f s), 1000000 runs, 56 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 56 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 57 msecs


After:

Benchmarking with V8
[s nil f clojure.string/blank?], (f s), 1000000 runs, 27 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 26 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 27 msecs
Benchmarking with SpiderMonkey
[s nil f clojure.string/blank?], (f s), 1000000 runs, 262 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 262 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 261 msecs
Benchmarking with JavaScriptCore
[s nil f clojure.string/blank?], (f s), 1000000 runs, 5 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 4 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 5 msecs
Benchmarking with Nashorn
[s nil f clojure.string/blank?], (f s), 1000000 runs, 328 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 324 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 60 msecs
Benchmarking with ChakraCore
[s nil f clojure.string/blank?], (f s), 1000000 runs, 62 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 63 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 62 msecs
Lindas-iMac-2:clojurescript mfikes$ # Using or nil?


 Comments   
Comment by Mike Fikes [ 05/Aug/17 11:37 AM ]

The change in the attached patch was used to produce the benchmarks in the ticket description.

I also tried an alternative which uses a Clojure or and a nil? check in lieu of the goog.string/makeSafe call, and this resulted in the same benchmark numbers.

So, the patch goes with the recommended deprecation change in the documentation for goog.string/isEmptyOrWhitespaceSafe, which indicates "Use goog.string.isEmptyOrWhitespace(goog.string.makeSafe(str)) instead.", which exactly matches the code we are indirectly calling today.





[CLJS-2268] clojure.string/escape in ClojureScript (unlike in Clojure) assumes cmap is a map Created: 23/Jul/17  Updated: 29/Oct/17

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

Type: Defect Priority: Minor
Reporter: Max Kreminski Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: cljs

Attachments: Text File 0001-CLJS-2268-Make-clojure.string-escape-constent-with-C.patch    
Patch: Code and Test

 Description   

The ClojureScript implementation of the clojure.string/escape function assumes that the cmap parameter will always be a map. This makes it different from (and specifically less general than) the Clojure implementation of the same function, which permits cmap to be anything callable.

Here's the relevant lines of the clojure.string/escape implementations in Clojure and ClojureScript. The ClojureScript implementation calls get on cmap, while the Clojure implementation invokes cmap directly.

Here's an example that works on Clojure, but doesn't work on ClojureScript, because it passes a function to clojure.string/escape instead of a map:

(defn regex-escape
  "Escapes regex special chars in the string `s`."
  [s]
  (let [special? #{\- \[ \] \{ \} \( \) \* \+ \? \. \\ \^ \$ \|}]
    (clojure.string/escape s #(when (special? %) (str \\ %)))))

Ideally, this discrepancy would be fixed by changing the ClojureScript implementation of clojure.string/escape to follow the Clojure one. This would also match the behavior described in the function's docstring, which is the same on both platforms.






[CLJS-2342] Speed up printing of js object by using forEach and regex optimizations Created: 30/Aug/17  Updated: 29/Oct/17

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: performance

Attachments: Text File CLJS-2342-2.patch     Text File CLJS-2342-3.patch     Text File CLJS-2342.patch    
Patch: Code

 Description   

By using goog.object/forEach, along with direct interop for regex checking it is possible to speed up the printing of JavaScript objects anywhere between 1.14 and 1.77



 Comments   
Comment by Mike Fikes [ 30/Aug/17 10:50 AM ]
Speedup Summary:

        Engine  small, medium, with sub-object
            V8: 1.42, 1.31, 1.19
  SpiderMonkey: 1.14, 1.48, 1.41
JavaScriptCore: 1.48, 1.58, 1.62
       Nashorn: 1.27, 1.36, 1.20
    ChakraCore: 1.49, 1.77, 1.77


Before:

Benchmarking with V8
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 74 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 84 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 75 msecs

Benchmarking with SpiderMonkey
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 95 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 92 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 134 msecs

Benchmarking with JavaScriptCore
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 46 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 41 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 55 msecs

Benchmarking with Nashorn
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 1228 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 1048 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 620 msecs

Benchmarking with ChakraCore
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 55 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 76 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 129 msecs


After:

Benchmarking with V8
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 52 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 64 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 63 msecs

Benchmarking with SpiderMonkey
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 83 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 62 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 95 msecs

Benchmarking with JavaScriptCore
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 31 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 26 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 34 msecs

Benchmarking with Nashorn
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 970 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 769 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 518 msecs

Benchmarking with ChakraCore
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 37 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 43 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 73 msecs
Comment by Mike Fikes [ 01/Oct/17 9:27 AM ]

Added CLJS-2342-2.patch which rebases against current master.

Comment by Mike Fikes [ 29/Oct/17 9:03 AM ]

CLJS-2342-3.patch rebaselines against current master.





[CLJS-1627] jsdoc parsing fails to recognize union types, breaking resolution Created: 18/Apr/16  Updated: 19/Nov/17

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

Type: Defect Priority: Minor
Reporter: Patrick Killean Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: patch

Attachments: Text File CLJS-1627-4.patch     Text File CLJS-1627-5.patch    
Patch: Code and Test

 Description   

The Closure Spec For Union Types states that parentheses are necessary for union type expressions. Trying this ...

(defn foo
  "@param {(IBar|IMap)} x"
  [x] 
  ...)

Raises a Closure Error :

...ERROR - Bad type annotation. expected closing }
* @param {user.(IBar|user.IMap)}

This is because comp/resolve-types treats the parentheses as a part of the type tokens and incorrect var resolution occurs as a result. In addition, the compiler emits multiple resolved types separated by "|" characters but does not enclose them in parentheses to create a valid union type.



 Comments   
Comment by Patrick Killean [ 18/Apr/16 4:36 PM ]

This patch includes:

  • comp/resolve-types now removes parentheses when present and emits them when >1 type is detected. This makes parenthesis use optional and existing code remains unbroken (with the added benefit that it may work now)
  • changes to comp/resolve-type
    1. checks for js globals like document or window which are recognized by closure
    2. allows dot.delimited.forms to pass through so we can use types defined in externs and avoid unnecessary resolution
    3. uses ana/resolve-existing-var with a "unresolved jsdoc type" warning
    4. checks if a resolved var is a protocol and warns otherwise. This is more informative than Closure's standard unrecognized type error
  • a test for comp/resolve-types
Comment by David Nolen [ 21/Apr/16 12:45 PM ]

Thanks will try to look more closely at this tomorrow.

Comment by David Nolen [ 23/Apr/16 2:03 PM ]

The patch is getting there, please remove the `js-doc-type` meta stuff. Just extend the signature of resolve-existing-var to take an additional parameter - the confirm-var-exists handler.

Comment by Patrick Killean [ 09/May/16 4:58 PM ]

CLJS-1627-1.patch:
resolve-existing-var now has an additional arity that accepts a missing-var handler passed to confirm-existing-var

Comment by Patrick Killean [ 10/May/16 6:16 AM ]

This has revealed a problem where deftype + defrecord using Object protocols emit resolved names when really they shouldn't. For example : "@implements {cljs.core.async.impl.timers.Object}" --> Bad Type Annotation

Since Object is a special case simply excluding it from the comments should fix it. Another patch incoming

Comment by Patrick Killean [ 10/May/16 7:42 AM ]

CLJS-1627-2.patch:
The emit* methods for deftype and defrecord now filter out Object protocols.

This produced an interesting result! With no more bad type annotations, static analysis can now proceed... and it has alot to say. Theres all kinds of info now about arity discrepencies (particularly cljs.core.IndexedSeq), type mismatches, and more. It even includes a type coverage percentage. Lots to parse here but very cool.

Comment by Patrick Killean [ 18/May/16 4:26 PM ]

CLJS-1627-3.patch:

  • fix require extern
  • add type application support for Array & Object
  • GC likes uppercase for Object & Array, lowercase for string, number.
  • support for explicit nullable types, variable typed arg
  • function type context modifiers this + new

Missing is the GC 'record type' . It also may be useful to fill out the node externs for common types

Comment by Patrick Killean [ 20/May/16 11:42 AM ]

CLJS-1627-4.patch:

  • fix a few problems in last patch
  • add record type support. Everything here should be covered
Comment by Patrick Killean [ 02/Sep/16 8:21 AM ]

update patch

Comment by Mike Fikes [ 19/Nov/17 7:55 PM ]

CLJS-1627-5.patch no longer applies





[CLJS-1997] Outward function type hint propagation Created: 03/Apr/17  Updated: 20/Nov/17

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File CLJS-1997.patch    
Patch: Code

 Description   

New feature: Detect the type of a function and arrange so that it is available for outward type hinting.

For reference, a small amount of discussion from Jan 2016 in the #cljs-dev Slack:

mfikes: Curious if "outward" ^boolean propagation has been debated / rejected. Example: If simple analysis detects that ^boolean can be applied as in for functions like (defn f [x] (not x)). Perhaps automatically propagating type hints in this direction would be unwelcome by users?

dnolen: @mfikes: we have already have outward propagation for numerics which broke because of changes for code motion

dnolen: absolutely nothing against that



 Comments   
Comment by Mike Fikes [ 03/Apr/17 7:38 PM ]

The attached patch will handle cases like

(defn foo? [x] (or (string? x) (number? x)))

and

(def baz (fn ([x] 1) ([x y] 2)))

but it doesn't attempt to address the case of multi-arity defn s, as these are implemented by dispatching to multiple single-arity functions that could have different return types.

It adds a :inferred-ret-tag to the AST only because the arguments to or on line 18 in the patch are not reversed (only to allow explicit hints to override inferred hints, but perhaps that is overcomplicating things and the inferred tag could be dropped directly into :ret-tag.)

Bronsa helpfully pointed out in Slack that outward type hint propagation is avoided in Clojure because it breaks things like:

(defn foo [] 1)
(defn bar [] (String/valueOf (foo)))
(defn foo [] "foo")
(bar)

for which an analogy in ClojureScript would be:

(defn foo? [] true)
(defn bar [] (if (foo?) :t :f))
(defn foo? [] "")
(bar)

where the patch causes it to yield :f.

This could be viewed as a REPL-only concern (perhaps addressed by another compiler flag along the lines of :static-fns).

But, arguably the problem already exists today, with type hinting in code like the following affecting the compilation of bar:

(def foo? true)
(defn bar [] (if foo? :t :f))
(def foo? "")
(bar)
Comment by David Nolen [ 07/Apr/17 11:15 AM ]

Thanks will think about it

Comment by Mike Fikes [ 16/Jun/17 10:30 AM ]

I've been soaking this for a couple of months (with local builds of Planck), and haven't seen anything break, FWIW. So, I think it is at least a safe change. Assigning to David for further consideration.





[CLJS-1975] Perf: Compare f with nil in Delay impl Created: 11/Mar/17  Updated: 21/Nov/17

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: performance

Attachments: Text File CLJS-1975.patch    
Patch: Code

 Description   

When a Delay has been realized, f is set to nil. We can avoid truth_ and not calls and directly compare with nil for a minor perf boost.

In script/noderepljs, this leads to these

(simple-benchmark [x (delay 1)] @x 1e9)
(simple-benchmark [x (delay 1)] (realized? x) 1e9)
(simple-benchmark [x (doto (delay 1) deref)] (realized? x) 1e9)

speeding up by 6%, 11% and 9%, respectively.



 Comments   
Comment by Mike Fikes [ 21/Nov/17 2:35 PM ]

Under :advanced across all engines, the 3 benchmarks in the ticket description produce these speedup numbers:

            V8: 1.18, 1.15, 1.09
  SpiderMonkey: 1.08, 1.04, 1.19
JavaScriptCore: 1.70, 0.86, 0.93
       Nashorn: 1.05, 1.15, 1.14
    ChakraCore: 1.21, 0.52, 0.81




[CLJS-2383] Improve perf of select-keys by using keyword-identical? Created: 17/Oct/17  Updated: 19/Nov/17

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: performance

Attachments: Text File 0001-CLJS-2383-Speed-up-select-keys.patch     Text File 0002-CLJS-2383-Speed-up-select-keys-no-transient.patch     Text File CLJS-2383.patch    
Patch: Code and Test

 Description   

select-keys uses not= to compare keywords. Instead, using keyword-identical? results in decent speedups (an average of 1.34 across benchmarks and engines). Note that using identical? and lookup-sentinel doesn't appear to improve perf.

Speedup Summary:

V8: 1.15, 1.08, 1.08
  SpiderMonkey: 1.71, 1.48, 1.67
JavaScriptCore: 1.33, 1.35, 1.25
       Nashorn: 1.16, 1.04, 0.97
    ChakraCore: 1.59, 1.66, 1.72

Before:

Benchmarking with V8
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 179 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 121 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 183 msecs

Benchmarking with SpiderMonkey
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 251 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 201 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 290 msecs

Benchmarking with JavaScriptCore
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 112 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 73 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 119 msecs

Benchmarking with Nashorn
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 1277 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 524 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 635 msecs

Benchmarking with ChakraCore
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 463 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 268 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 414 msecs

After

Benchmarking with V8
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 155 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 112 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 169 msecs

Benchmarking with SpiderMonkey
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 146 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 135 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 173 msecs

Benchmarking with JavaScriptCore
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 84 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 54 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 95 msecs

Benchmarking with Nashorn
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 1099 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 502 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 648 msecs

Benchmarking with ChakraCore
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 292 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 151 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 240 msecs


 Comments   
Comment by Erik Assum [ 17/Oct/17 2:37 PM ]

Just want to bring your attention to CLJ-1789, where reimplementing `select-keys` in terms of reduce gave a significant speedup.
Added a patch to show that way.

Comment by Mike Fikes [ 18/Oct/17 6:46 AM ]

Here are the perf numbers for Erik's patch:

V8: 0.81, 1.14, 1.30
  SpiderMonkey: 1.49, 1.31, 1.58
JavaScriptCore: 1.02, 0.99, 0.96
       Nashorn: 1.13, 1.17, 1.21
    ChakraCore: 1.27, 1.35, 1.28

After:

Benchmarking with V8
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 220 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 106 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 141 msecs

Benchmarking with SpiderMonkey
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 169 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 153 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 183 msecs

Benchmarking with JavaScriptCore
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 110 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 74 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 124 msecs

Benchmarking with Nashorn
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 1128 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 447 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 524 msecs

Benchmarking with ChakraCore
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 366 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 199 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 323 msecs
Comment by Erik Assum [ 18/Oct/17 6:56 AM ]

Uploaded a patch without transients as well.

Comment by Mike Fikes [ 18/Oct/17 7:40 AM ]

Since the CLJ-1789 patch works better with larger maps, here is an additional perf test covering that case using the data from that ticket, testing both the original patch I had attached and Erik's subsequent patch. You can see the CLJ-1789 approach pays off for ClojureScript as well.

Erik, I see you attached a 3rd patch. I'd recommend adding perf numbers with each such patch, so the effect of the patch under advanced optimizations can be more readily assessed.

Engine          keyword-identical?  CLJ-1789
            V8:               1.13      1.29
  SpiderMonkey:               1.89      2.39
JavaScriptCore:               1.02      0.96
       Nashorn:               1.12      1.42
    ChakraCore:               1.68      1.82

Before:

Benchmarking with V8
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 373 msecs

Benchmarking with SpiderMonkey
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 668 msecs

Benchmarking with JavaScriptCore
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 200 msecs

Benchmarking with Nashorn
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 2236 msecs

Benchmarking with ChakraCore
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 1074 msecs

After (keyword-identical?)

Benchmarking with V8
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 330 msecs

Benchmarking with SpiderMonkey
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 353 msecs

Benchmarking with JavaScriptCore
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 197 msecs

Benchmarking with Nashorn
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 1991 msecs

Benchmarking with ChakraCore
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 640 msecs

After (CLJ-1789)

Benchmarking with V8
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 290 msecs

Benchmarking with SpiderMonkey
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 279 msecs

Benchmarking with JavaScriptCore
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 209 msecs

Benchmarking with Nashorn
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 1578 msecs

Benchmarking with ChakraCore
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 591 msecs
Comment by Erik Assum [ 18/Oct/17 7:54 AM ]

Both patches should have benchmarks now

Comment by Erik Assum [ 18/Oct/17 7:56 AM ]

oh, and as a comment to the comment about larger maps, I believe the performance `transient` bit is dependent on the size of the selected keys,
eg the more selected keys found in the map, the more we gain from `conj!`

Comment by Mike Fikes [ 23/Oct/17 12:04 PM ]

Running these 4 benchmarks

[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs

against all 5 engines with the 3 attached patches yields the following speedups on my machine:

CLJS-2383.patch:

            V8: 1.11, 1.10, 1.64, 1.18  Avg: 1.26
  SpiderMonkey: 2.36, 1.46, 1.79, 2.02  Avg: 1.91
JavaScriptCore: 1.28, 1.34, 1.23, 1.49  Avg: 1.34
       Nashorn: 1.19, 1.17, 1.06, 1.29  Avg: 1.18
    ChakraCore: 1.61, 1.78, 1.75, 2.11  Avg: 1.81
                                Overall avg: 1.50
         Avg excluding Nashorn & ChakraCore: 1.50

0001-CLJS-2383-Speed-up-select-keys.patch:

            V8: 0.70, 0.95, 1.05, 1.23  Avg: 0.98
  SpiderMonkey: 1.20, 1.09, 1.05, 2.03  Avg: 1.34
JavaScriptCore: 0.78, 0.84, 0.83, 1.02  Avg: 0.87
       Nashorn: 1.18, 1.08, 1.02, 1.48  Avg: 1.19
    ChakraCore: 1.00, 1.12, 1.19, 1.75  Avg: 1.27
                                Overall avg: 1.13
         Avg excluding Nashorn & ChakraCore: 1.06	

0002-CLJS-2383-Speed-up-select-keys-no-transient.patch:
	
            V8: 1.28, 1.45, 1.37, 1.33  Avg: 1.36
  SpiderMonkey: 1.54, 1.44, 1.70, 2.17  Avg: 1.71
JavaScriptCore: 1.01, 0.99, 1.03, 1.13  Avg: 1.04
       Nashorn: 1.39, 1.21, 1.14, 1.26  Avg: 1.25
    ChakraCore: 1.20, 1.23, 1.19, 1.39  Avg: 1.25
                                Overall avg: 1.32
         Avg excluding Nashorn & ChakraCore: 1.37
Comment by Mike Fikes [ 19/Nov/17 7:08 PM ]

Summary: If applied, CLJS-2383.patch would be the one to apply as it provides the greatest speedup of all the patches.





[CLJS-2346] Make (:require foo/bar) work for JS modules Created: 01/Sep/17  Updated: 05/Sep/17

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

Type: Enhancement Priority: Minor
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: js-modules

Attachments: Text File CLJS-2346.patch    
Patch: Code

 Description   

the string require only really becomes necessary when there are multiple slashes. We can support this style as a symbol for dependencies that only have 1 slash because it's a valid symbol.



 Comments   
Comment by Thomas Heller [ 02/Sep/17 2:43 AM ]

Me again ... what is so bad about using string requires? Qualified symbols are illegal for normal CLJS code and using a string already solves all potential issues. Adding a case where you can use a symbol if there is one slash but not if there are two is just going to confuse new users with no additional benefit.

Comment by Juho Teperi [ 05/Sep/17 3:40 AM ]

I can see the point of only allowing slashes with strings.

However, if we do this, it might be good to check the warnings we give when trying to do this:

(ns example.core
  (:require [react-dom/server :as sdf]))

  No such namespace: react-dom/server, could not locate react_dom_SLASH_server.cljs, react_dom_SLASH_server.cljc, or JavaScript source providing "server"

Notice how it says "server" instead of react-dom/server. This is because everything allows slashes, but foreign lib code only uses name part instead of namespace. (This patch doesn't change the warning.)

Comment by Thomas Heller [ 05/Sep/17 4:50 AM ]

There are other ambiguities when it comes to JS requires which is why I'm still advocating for just using strings in all circumstances.

Technically we could make {{(:require [@scoped/thing :as thing])}} work as well but shouldn't.

Also I'm pretty sure I saw several JS packages that either used "-" or "_" in their names. For CLJS we always convert to an underscore which would add more confusion if we don't do this for JS.

Yes, the warning should be fixed but wouldn't even be an issue if a string was used.





[CLJS-2132] Optimize transient vector creation Created: 27/Jun/17  Updated: 27/Jun/17

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

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: performance

Attachments: Text File CLJS-2132.patch    
Patch: Code

 Description   

This is a very simple optimization around transient []. It avoids copying the empty array.

Performance improvements, for mapv on smallish vectors (5-32) elements anywhere from 20% up to 100% across FF & Chrome.

(defn faster-editable-root
  [node]
  (if (identical? (.-EMPTY_NODE PersistentVector) node)
    (VectorNode. (js-obj) (make-array 32))
    (VectorNode. (js-obj) (aclone (.-arr node)))))
(def orig-editabe-root tv-editable-root)
(enable-console-print!)
(dotimes [_ 2]
  (doseq [size [5 10 40]]
    (let [xs (range size)
          sims 500000]
      (set! tv-editable-root orig-editabe-root)
      (prn "Size: " size)
      (simple-benchmark [] (mapv inc xs) sims)
      (set! tv-editable-root faster-editable-root)
      (prn "NEW:")
      (simple-benchmark [] (mapv inc xs) sims))))





[CLJS-2045] sort-by: Avoid re-creating comparator Created: 20/May/17  Updated: 25/Jun/17

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

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File CLJS-2045.patch    
Patch: Code

 Description   

The fn->comparator call should be lifted:

(sort (fn [x y] ((fn->comparator comp) (keyfn x) (keyfn y))) coll)
(let [comparator (fn->comparator comp)]
  (sort (fn [x y] (comparator (keyfn x) (keyfn y))) coll))

Also, fn->comparator is again called on the function in sort, not sure how to avoid that unless we copy the sort code into sort-by.






[CLJS-1601] Optimize cljs.core dump Created: 12/Mar/16  Updated: 19/Nov/17

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

Type: Enhancement Priority: Minor
Reporter: Nikita Beloglazov Assignee: David Nolen
Resolution: Unresolved Votes: 2
Labels: bootstrap

Attachments: Text File CLJS-1601.patch     Text File CLJS-1601.patch    
Patch: Code

 Description   

When building cljs source that uses cljs.js namespace the final js file is quite huge: 6.4M. As described in wiki: https://github.com/clojure/clojurescript/wiki/Optional-Self-hosting it mostly consists of analysis cache of the cljs.core namespace. As a workaround, the wiki article suggests dumping cache to a separate file and load it at runtime instead of bundling in js binary. I think it is possible to have something in between that doesn't require additional efforts from a user and also optimizes the size of the js file. The idea that instead of dumping cache as raw clojure data-structure it is serialized to string. This way compiler won't compile cache into js (which adds a lot of code) and leave it a string. At runtime, this string will be parsed back to clojure using tools.reader.

Here is the proposal: https://gist.github.com/nbeloglazov/0bf163fb62fa4b61d446

Checking locally it reduces the size of js file from 6.4M to 2.7M which I think quite good. The downside is that now js has to do more work on runtime (parse huge string) when today it simply read js code and evaluates it. But I don't think if it's a big concern. If it is desired to keep all behavior a new option can be added for :dump-core compiler setting, something like :dump-core :string that enables string serialization of the cache.

Does it sound reasonable?



 Comments   
Comment by Nikita Beloglazov [ 27/Mar/16 8:54 PM ]

Attaching suggested fix. Analysis cache is serialized to string and read back to clojure datastructure when cljs.js is initialized.

Comment by David Nolen [ 28/Mar/16 6:39 AM ]

Please change the patch so this optional as you've suggested.

Comment by David Nolen [ 28/Mar/16 6:40 AM ]

Also have you submitted your Clojure CA yet?

Comment by Nikita Beloglazov [ 28/Mar/16 1:35 PM ]

Will do. Yes, I've submitted CA. I used my official name, Mikita Belahlazau there.

Comment by Nikita Beloglazov [ 29/Mar/16 12:16 AM ]

Updated patch that adds option to serialize core analysis cache as string. Possible values of :dump-core are :raw, :string, :none. Old true/false values supported for backward compatibility.

As for default, current patch uses :raw, but I think it makes more sense to use :string. Saving extra few mb of final js is quite good. I think most devs won't go deep into figuring out why js is big and just leave it as it is. Additional one-time parsing performance hit :string introduces acceptable: when :string is used, page loads in 1s while with :raw the time is ~800ms.

Comment by David Nolen [ 16/Jun/17 12:41 PM ]

I'm questioning whether this actual valuable? It seems to me if you're serious about code size you would just use Transit and them load the analysis asynchronously?

Comment by Nikita Beloglazov [ 17/Jun/17 2:39 AM ]

Yes, if size is critical then there are better ways to hand-tune the way of loading analysis. At the same time having 3m vs 6m file for local/simple development is a nice win. The only downside is speed, but I feel like big reduction in size is better than small speed penalty.

Comment by Mike Fikes [ 19/Nov/17 7:56 PM ]

Patch no longer applies.





[CLJS-1587] Duplicate keys via quoting Created: 24/Feb/16  Updated: 19/Nov/17

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

Type: Defect Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: None

Attachments: File CLJS-1587.diff    
Patch: Code and Test

 Description   

For

#{1 '1}

you get

#{1 1}


 Comments   
Comment by Peter Schuck [ 03/Mar/16 10:01 PM ]

This happens for the has-set macro and the hash-set literal. Here's what I get from the repl

cljs.user=> (hash-set 1 '1 2 '2 3 '3 4 '4 5)
#{1 2 3 4 5}
cljs.user=> (hash-set 1 '1 2 '2 3 '3 4 '4)
#{1 1 2 2 3 3 4 4}
cljs.user=> #{ 1 '1 2 '2 3 '3 4 '4}
#{2 1 4 4 3 2 1 3}
cljs.user=> #{ 1 '1 2 '2 3 '3 4 '4 5}
#{2 1 4 4 3 2 5 1 3}
cljs.user=> #{ 1 '1 2 '2 3 '3 4 '4 5 '5}
#{2 5 1 4 4 3 2 5 1 3}
cljs.user=> (apply hash-set [1 '1 2 '2 3 '3 4 '4])
#{1 2 3 4}

Calling hash-set as a function gives the correct results. The hash-set macro gives the incorrect results until we have more then 8 elements and uses the fromArray method on PersistentHashSet to build the set instead of creating a literal PersistentArrayMap for the set. The literal notation is incorrect no matter how many elements there are.

Comment by Rohit Aggarwal [ 15/Jun/16 8:02 AM ]

The underlying problem for both is the same in that a PersistentHashSet is being created directly using a PersistentArrayMap where the keys are the elements from the provided sequence. It manifests itself in two places though.

Comment by Rohit Aggarwal [ 15/Jun/16 9:32 AM ]

I've taken the approach that if we see a quoted constant then don't create the PersistentHashSet directly and instead go via the fromArray function.

Patch has the fix and a test.

Comment by Mike Fikes [ 13/Jun/17 9:40 PM ]

Attached patch no longer applies to master.

Comment by A. R [ 14/Jun/17 1:43 AM ]

It'd be nice if this patch/ticket also included the following case:

(hash-set "a" \a)
Comment by A. R [ 14/Jun/17 10:50 AM ]

Should we increase the scope of this ticket? The same issue exists for maps:

{'0 "a", 0 "b"}
{\a "a", "a" "b"}

I think one possible solution that solves both, quoting and the char/string, could be to to call emit-str in cljs.compiler on the keys/set-members and only then check for uniqueness. Not sure that's a good idea though.

Doesn't solve the hash-set, array-map macros.

Edit: Related ticket: CLJS-2087

Comment by David Nolen [ 14/Jun/17 1:45 PM ]

Increasing the scope of tickets is not desirable. Please move to a separate issue and cross-reference thanks.

Comment by Mike Fikes [ 19/Nov/17 7:57 PM ]

Patch no longer applies.





[CLJS-1474] Error if reserved symbol is defined Created: 21/Oct/15  Updated: 31/Jul/16

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

Type: Enhancement Priority: Minor
Reporter: Martin Klepsch Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: newbie

Attachments: Text File CLJS-1474.patch    
Patch: Code and Test

 Description   

Currently a definition like

(defn set! [] ...)

will not cause any warning. Any usage of it (without :as namespace aliasing) however will not use the defined var but the set! special form.

A warning seems appropriate.



 Comments   
Comment by António Nuno Monteiro [ 30/Jul/16 1:19 PM ]

Attached patch with fix and test.

Comment by Mike Fikes [ 31/Jul/16 2:09 PM ]

I know David suggested that a hard error is probably the right thing to do for this one, but one consequence is that the cljs.spec/def macro cannot be defined in bootstrap with this change. (I haven't investigated thoroughly, but this may simply be the result of macros being processed as ClojureScript in bootstrap, and thus being subject to this new guard.)

Regardless of the root cause, you'll see this if you try to run script/test-self-parity:

#error {:message "Could not eval cljs.spec", :data {:tag :cljs/analysis-error}, :cause #error {:message "Can't def special form at line 51 ", :data {:file nil, :line 51, :column 1, :tag :cljs/analysis-error}}}

For reference: Line 51 currently points at the def macro: https://github.com/clojure/clojurescript/blob/e2db5d9ff8cb6a099ebc2a8cd379385bf4649b38/src/main/cljs/cljs/spec.cljc#L51





[CLJS-994] print a warning when :externs file paths can't be found. Created: 30/Jan/15  Updated: 19/Nov/17

Status: Reopened
Project: ClojureScript
Component/s: None
Affects Version/s: 1.7.145
Fix Version/s: 1.9.671

Type: Enhancement Priority: Minor
Reporter: Crispin Wellington Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: cljs, enhancement, errormsgs, patch,
Environment:

Linux 64bit

java version "1.7.0_65"
OpenJDK Runtime Environment (IcedTea 2.5.3) (7u71-2.5.3-0ubuntu0.14.04.1)
OpenJDK 64-Bit Server VM (build 24.65-b04, mixed mode)


Attachments: Text File clojurescript-extern-missing-warning.patch    
Patch: Code

 Description   

clojurescript silently ignores missing externs files possibly leading a developer to chase their tail.

Presently it can be very confusing using advanced compilation if you have made a mistake in the path name of one of your :externs files. This patch makes the compiler print a warning on stderr so you can quickly determine the cause of the broken advanced compilation output.

As a side effect, when doing a basic lein-cljsbuild a warning is always printed:

```
WARNING: js resource path closure-js/externs does not exist
```

This is because lein-cljsbuild quietly adds this extra path to your :externs listing without you knowing.



 Comments   
Comment by David Nolen [ 31/Jan/15 1:59 PM ]

You need to bind *out* to *err*, or just print to it directly a la cljs.util/debug-prn.

Comment by Crispin Wellington [ 31/Jan/15 7:30 PM ]

I did bind out to err. Check the patch.

Comment by David Nolen [ 01/Feb/15 12:30 PM ]

Crispin, oops sorry you are correct. Thanks.

Comment by David Nolen [ 13/Mar/15 7:33 AM ]

fixed https://github.com/clojure/clojurescript/commit/5f66a78bf469a9875e51aa39c29d3e66ce890eb4

Comment by David Nolen [ 14/Mar/15 5:55 AM ]

The solution does not work for cljsbuild. It's unclear why there so much machinery in place over the approach taken for deps.clj.

Comment by David Nolen [ 15/Mar/15 10:37 AM ]

Stalled on this cljsbuild issue https://github.com/emezeske/lein-cljsbuild/issues/383

Comment by Crispin Wellington [ 23/Mar/15 2:50 AM ]

This lein-cljsbuild issue is what made me make it just a warning initially, and not a hard error like raising IllegalArgumentException does. Though I agree it should be a hard error. If we start with a warning, it enables the immediate problem for the developer to be resolved, and leaves a wart that the cljs-build project can then see that need fixing on their end. Then when that end is fixed it could be made a hard error. If cljsbuild is fixed fairly soon then all is well, but if it takes a long time, a warning might be a good first step.

Comment by Mike Fikes [ 19/Nov/17 8:06 PM ]

Patch no longer applies. (Also it doesn't work with git am—see https://clojurescript.org/community/patches.)





[CLJS-2341] Speed up js->clj on objs using forEach and transient volatile Created: 30/Aug/17  Updated: 31/Aug/17

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 3
Labels: performance

Attachments: Text File CLJS-2341.patch    
Patch: Code

 Description   

It is possible to speed up js->clj on JavaScript objects by revising the implementation to employ goog.object/forEach, accumulating by bashing on a transient volatile map.

The speedups range from 1.5 to 2.1 over the current implementation.

Note: The current implementation uses js-keys. The optimization in CLJS-2340 could help js->clj, but it doesn't appear to provide much speedup in itself (perhaps 1.1?) compared to the change in implementation described above.



 Comments   
Comment by Mike Fikes [ 30/Aug/17 9:19 AM ]
Speedup Summary:

        Engine  small, medium, with sub-object
            V8: 1.62, 1.50, 1.13
  SpiderMonkey: 1.91, 1.74, 1.59
JavaScriptCore: 1.67, 1.74, 2.10
       Nashorn: 1.54, 2.13, 1.51
    ChakraCore: 1.71, 2.10, 1.95


Before:

Benchmarking with V8
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 55 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 63 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 93 msecs

Benchmarking with SpiderMonkey
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 155 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 87 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 94 msecs

Benchmarking with JavaScriptCore
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 45 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 33 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 42 msecs

Benchmarking with Nashorn
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 1123 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 1195 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 773 msecs

Benchmarking with ChakraCore
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 65 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 44 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 78 msecs


After:

Benchmarking with V8
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 34 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 42 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 82 msecs

Benchmarking with SpiderMonkey
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 81 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 50 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 59 msecs

Benchmarking with JavaScriptCore
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 27 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 19 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 20 msecs

Benchmarking with Nashorn
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 728 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 561 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 513 msecs

Benchmarking with ChakraCore
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 38 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 21 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 40 msecs




[CLJS-2120] Optimize keyword function Created: 24/Jun/17  Updated: 25/Jun/17

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

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 2
Labels: performance

Attachments: Text File CLJS-2120.patch    
Patch: Code
Approval: Accepted

 Description   

keyword function can be sped up. This matters when keywords are created dynamically when doing XHR.

The patch now matches Clojure more closely (using substring). It's also optimized for passing strings.

Results:

(enable-console-print!)
(let [sims 1000000]
  (dotimes [_ 2]
    (doseq [x ["foo" "foo/bar" [nil "foo"] ["foo" "bar"] [:foo :bar] [nil :foo]]]
      (prn "Testing keyword with: " x)
      (if (vector? x)
        (do (simple-benchmark [[a0 a1] x] (set! js/FOOO (keyword a0 a1)) sims)
            (simple-benchmark [[a0 a1] x] (set! js/FOOO (keyword2 a0 a1)) sims))
        (do (simple-benchmark [] (set! js/FOOO (keyword x)) sims)
            (simple-benchmark [] (set! js/FOOO (keyword2 x)) sims))))))


"Testing keyword with: " "foo"
[], (set! js/FOOO (keyword x)), 1000000 runs, 194 msecs
[], (set! js/FOOO (keyword2 x)), 1000000 runs, 71 msecs
"Testing keyword with: " "foo/bar"
[], (set! js/FOOO (keyword x)), 1000000 runs, 260 msecs
[], (set! js/FOOO (keyword2 x)), 1000000 runs, 104 msecs
"Testing keyword with: " [nil "foo"]
[[a0 a1] x], (set! js/FOOO (keyword a0 a1)), 1000000 runs, 278 msecs
[[a0 a1] x], (set! js/FOOO (keyword2 a0 a1)), 1000000 runs, 188 msecs
"Testing keyword with: " ["foo" "bar"]
[[a0 a1] x], (set! js/FOOO (keyword a0 a1)), 1000000 runs, 379 msecs
[[a0 a1] x], (set! js/FOOO (keyword2 a0 a1)), 1000000 runs, 215 msecs
"Testing keyword with: " [:foo :bar]
[[a0 a1] x], (set! js/FOOO (keyword a0 a1)), 1000000 runs, 351 msecs
[[a0 a1] x], (set! js/FOOO (keyword2 a0 a1)), 1000000 runs, 207 msecs
"Testing keyword with: " [nil :foo]
[[a0 a1] x], (set! js/FOOO (keyword a0 a1)), 1000000 runs, 376 msecs
[[a0 a1] x], (set! js/FOOO (keyword2 a0 a1)), 1000000 runs, 37 msecs


 Comments   
Comment by A. R [ 24/Jun/17 10:56 AM ]

Changes the behavior of:

((juxt namespace name) (keyword "foo/bar/hmm"))
=> [nil "foo"]
(.-fqn (keyword "foo/bar/hmm"))
=> "foo/bar/hmm"
((juxt namespace name) (keyword2 "foo/bar/hmm"))
=> ["foo" "bar/hmm"]
(.-fqn (keyword2 "foo/bar/hmm"))
=> "foo/bar/hmm"

Clojure 1.9:

((juxt namespace name) (keyword "foo/bar/hmm"))
=> ["foo" "bar/hmm"]

So: yay





[CLJS-2004] Minor fix for test-simple script Created: 10/Apr/17  Updated: 12/Dec/17

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

Type: Enhancement Priority: Minor
Reporter: Dejan Josifovic Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: clojurescript, script, test
Environment:

Lubuntu 16.10


Attachments: Text File CLJS-2004.patch     Text File CLJS-2004-rebase.patch    
Patch: Code

 Description   

On Ubuntu based Linux distributions $[] doesn't work.
Output is: Tested with $[ran+1] out of 4 possible js targets
and should be: Tested with 4 out of 4 possible js targets

As in CLJS-929 (for test script), $(()) will work for ash, dash, bash, and zsh.



 Comments   
Comment by Dejan Josifovic [ 10/Apr/17 2:42 PM ]

Add patch.

Comment by Mike Fikes [ 19/Nov/17 7:32 PM ]

Patch no longer applies; needs re-baseline.

Comment by Dejan Josifovic [ 12/Dec/17 5:33 PM ]

Hi,
Sorry for the late replay.
Rebased patch is uploaded.
Regards.





[CLJS-2409] Eliminate cljs.core/divide Created: 21/Nov/17  Updated: 14/Dec/17

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-2409-2.patch     Text File CLJS-2409-3.patch     Text File CLJS-2409.patch    
Patch: Code and Test

 Description   

The cljs.core/divide inlining macro was introduced to work around an inability to specify cljs.core//.

For example: https://github.com/clojure/clojurescript/blob/d45012273029bd5df3973ea716394f368e1c44cc/src/main/cljs/cljs/core.cljs#L2593

This can now evidently be removed and thus avoid things like CLJS-594 and divide showing up as a completion in IDEs, etc.



 Comments   
Comment by Mike Fikes [ 21/Nov/17 9:38 PM ]

CLJS-2409-2.patch adds unit tests.

Comment by Mike Fikes [ 14/Dec/17 2:34 PM ]

Adding re-baselined patch.





[CLJS-2038] self calls do not optimize - regression Created: 15/May/17  Updated: 25/Jun/17

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

Type: Defect Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None


 Description   

This is a regression of:

https://dev.clojure.org/jira/browse/CLJS-275



 Comments   
Comment by A. R [ 18/May/17 2:19 AM ]

This issue can be solve by changing the defn macro for the "simple" case and carrying the function name over to the function:

(core/list 'def (with-meta name m)
  (cons `fn (cons name fdecl)))

This isn't done in clojure because of: https://dev.clojure.org/jira/browse/CLJ-809

Though I don't think that's and issue in CLJS since we don't have real vars anyways and can't redefine {{def}}s.

Comment by A. R [ 17/Jun/17 10:07 AM ]

So the issue that the CLJ ticket has is emulated/shown below in CLJS:

(enable-console-print!)

(defn self-call-test
  [n]
  (prn "inner")
  (when (pos? n)
    (self-call-test (dec n))))

(let [orig self-call-test]
  (set! self-call-test
        (fn outer [n]
          (pr "outer")
          (orig n))))
(self-call-test 2)

(def self-call-test2
  (fn self-call-test2
       [n]
       (prn "inner")
       (when (pos? n)
         (self-call-test2 (dec n)))))

(let [orig self-call-test2]
  (set! self-call-test2
        (fn outer [n]
          (pr "outer")
          (orig n))))
(self-call-test2 2)

Output in with no optimizations:

"outer"
"inner"
"outer"
"inner"
"outer"
"inner"


"outer"
"inner"
"inner"
"inner"

So: It does seem this would also break the current behaviour, HOWEVER, the above with advance optimizations gives this:

"outer"
"inner"
"inner"
"inner"

*for both*. Given this, it seem better to not change behavior during advanced builds to avoid hard to track down production bugs for the users. Even if this is a slight deviation from CLJ behavior. Thoughts?

Comment by A. R [ 25/Jun/17 2:27 PM ]

Any thoughts on this? I can create a patch if that this change is ok. It could matter a bit (performance wise) since a few of the very low level data structure functions are recursive.





[CLJS-2424] Improve compiler munge performance Nr 2 Created: 28/Nov/17  Updated: 22/Dec/17

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

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 2
Labels: performance

Attachments: Text File CLJS-2424.patch    
Patch: Code

 Description   

This is similar to CLJS-2065 and further improves the performance by avoiding reduce and using a key iterator instead.

Results for a large CLJS project with lots of namespaces are:

  • Initial compile (cold) Old: 11.4s New: 11.2s
  • First full recompile: Old: 6.8s New: 5.9s
  • After a few full recompiler (warmed up JVM): Old: ~6.1s New: 5.1s

lein count:

Ext Files Lines of Code Nodes
cljs 138 23388 424745


 Comments   
Comment by David Nolen [ 22/Dec/17 2:24 PM ]

No difference for compiling ClojureScript tests, I will give a try with something else.





[CLJS-1965] letfn collisions across namespaces Created: 02/Mar/17  Updated: 05/Jan/18

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

Type: Defect Priority: Minor
Reporter: Jeremy R Sellars Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: None
Environment:

Affects 1.9.495 and earlier. Probably only affects browsers (not nodejs).


Attachments: Text File CLJS-1965-failing-tests.patch     Text File cljs-1965_wrap_letfn_statements.patch    
Patch: Code

 Description   

If you `letfn` a fn with the same name in two namespaces, the wrong fn is used.

one.cljs
(ns hello-world.one)

(letfn [(answer [] "1")]
  (defn get-answer []
    (answer)))
two.cljs
(ns hello-world.two)

(letfn [(answer [] "2")]
  (defn get-answer []
    (answer)))
core.cljs
(ns hello-world.core
  (:require [hello-world.one]
            [hello-world.two]))

(println "one =>" (hello-world.one/get-answer))  ; one => 1
(println "two =>" (hello-world.two/get-answer))  ; two => 1      WHAT?!?

This issue seems to exist both on top-level `letfn`s and non-top-level `(let [] (letfn [...]))`.



 Comments   
Comment by Jeremy R Sellars [ 02/Mar/17 4:21 PM ]

This patch wraps `letfn` :expr and :statement forms in a function declaration (formerly, only :expr forms were wrapped).

I only did minimal testing. It fixes the code from the description.

Note: This is my first crack at the compiler and it is entirely possible I have not understood the entire problem.

Comment by Mike Fikes [ 16/Jun/17 10:54 AM ]

Confirmed that this fixes things downstream in self-hosted ClojureScript (Planck):

Without the patch:

cljs.user=> (require 'hello-world.core)
one => 2
two => 2
nil

With the patch:

cljs.user=> (require 'hello-world.core)
one => 1
two => 2
nil
Comment by daniel sutton [ 29/Dec/17 3:18 PM ]

This doesn't require different namespaces. The bug is that `let-fn` is putting its binding as a global variable.

And easy reproduction is
1. `lein new mies letfn-bug`,
2. update cljs version to `[org.clojure/clojurescript "1.9.946"]`
3. and then

(ns letfn-bug.core
  (:require [clojure.browser.repl :as repl]))

(enable-console-print!)

(letfn [(non-unique-name [] 4)]
  (defn f1 [] (non-unique-name)))

(letfn [(non-unique-name [] 5)]
  (defn f2 [] (non-unique-name)))

(println "should be 4-> " (f1))
(println "should be 5-> " (f2))

then `scripts/repl`.

results in:

cljs.user=> (load-file "letfn_bug/core.cljs")
should be 4->  5
should be 5->  5
nil
cljs.user=>

With the generated js:

// Compiled by ClojureScript 1.9.946 {}
goog.provide('letfn_bug.core');
goog.require('cljs.core');
goog.require('clojure.browser.repl');
cljs.core.enable_console_print_BANG_.call(null);
var non_unique_name = (function letfn_bug$core$non_unique_name(){
return (4);
});
letfn_bug.core.f1 = (function letfn_bug$core$f1(){
return non_unique_name.call(null);
});
var non_unique_name = (function letfn_bug$core$non_unique_name(){
return (5);
});
letfn_bug.core.f2 = (function letfn_bug$core$f2(){
return non_unique_name.call(null);
});
cljs.core.println.call(null,"should be 4-> ",letfn_bug.core.f1.call(null));
cljs.core.println.call(null,"should be 5-> ",letfn_bug.core.f2.call(null));
Comment by David Nolen [ 05/Jan/18 10:42 AM ]

Quick review of the patch, instead of always wrapping in a statement context it might be better if we only did it at the top-level where this is actually a problem.





[CLJS-2459] Compiler should emit warning about namespace and/or non-existent var Created: 05/Jan/18  Updated: 06/Jan/18

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

Type: Enhancement Priority: Minor
Reporter: Michiel Borkent Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

Approval: Vetted

 Description   

Made project as described in quickstart.

hello-world/core.cljs

(ns hello-world.core
  (:require [hello-world.foo]))

(enable-console-print!)

(println "Hello world!")
(hello-world.foo/foo)
;; no warning

hello_world/foo.cljs

(ns hello-world.foo2)
(defn foo []
  (println "o no"))

Note that `foo.cljs` has a namespace name that is not consistent with the file structure.

I expect
1) a warning about `(:require [hello-world.foo])` not being able to find the namespace, and/or
2) a warning about `(hello-world.foo/foo)` being a non-existent var.






[CLJS-2466] Faster reduce for lazy-seqs Created: 12/Jan/18  Updated: 13/Jan/18

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

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File CLJS-2466.patch    

 Description   

Lazy seqs that are built from vectors/chunked-seqs can sometimes take a slow first/next reduce.

Observation:

(def xs (vec (range 3000000)))

(time (reduce max xs)) ;; #1: 130ms, (reference)
(time (reduce max (lazy-cat xs))) ;; #2: 130ms, just as fast
(time (reduce max 0 (lazy-cat xs))) ;; #3: 500ms, 4x slower!!

;; Double them with concat and they're not 2x slower but 10x slower:
(time (reduce max (lazy-cat xs xs))) ;; #4: 1200ms
(time (reduce max 0 (lazy-cat xs xs))) ;; #5: 1200ms

Explanation for #3: The problem is that seq-reduce when called without init will properly call reduce again and take a fast path. With init given it won't ever escape to a faster reduce but will stick to first/next.

Note: In Clojure they scale "properly" (first 3 are ~45ms, last 2 are ~110ms).

The reason is that Clojure properly escapes to a fast path:

https://github.com/clojure/clojure/blob/2b242f943b9a74e753b7ee1b951a8699966ea560/src/clj/clojure/core/protocols.clj#L131-L143

An improved seq-reduce could look like this:

(defn seq-reduce
  ([f coll]
   (if-let [s (seq coll)]
     (reduce f (first s) (next s))
     (f)))
  ([f val coll]
   (loop [val val, coll (seq coll)]
     (if coll
       (if (chunked-seq? coll)
         (if (implements? IReduce coll)
           (reduce f val coll)
           (let [nval (reduce f val (chunk-first coll))]
             (if (reduced? nval)
               @nval
               (recur nval (chunk-next coll)))))
         (let [nval (f val (first coll))]
           (if (reduced? nval)
             @nval
             (recur nval (next coll)))))
       val))))

This reduces the times for #3: 160ms, #4: 380ms, #5: 380ms.

This is an RFC, since I'm not 100% certain about the implemenation. Need to be careful to not blow the stack here...

Questions:
1. Should ChunkedCons implement IReduce? I think so.



 Comments   
Comment by A. R [ 12/Jan/18 1:59 AM ]

Timings with advanced compilation:

CHROME:
"OLD"
#1: "Elapsed time: 21.870000 msecs"
#2: "Elapsed time: 25.485000 msecs"
#3: "Elapsed time: 134.900000 msecs"
#4: "Elapsed time: 317.155000 msecs"
#5: "Elapsed time: 313.225000 msecs"
"NEW"
#1: "Elapsed time: 23.290000 msecs"
#2: "Elapsed time: 19.445000 msecs"
#3: "Elapsed time: 20.075000 msecs"
#4: "Elapsed time: 102.895000 msecs"
#5: "Elapsed time: 100.430000 msecs"
"OLD"
#1: "Elapsed time: 19.585000 msecs"
#2: "Elapsed time: 19.475000 msecs"
#3: "Elapsed time: 87.805000 msecs"
#4: "Elapsed time: 303.340000 msecs"
#5: "Elapsed time: 291.680000 msecs"
"NEW"
#1: "Elapsed time: 20.760000 msecs"
#2: "Elapsed time: 19.690000 msecs"
#3: "Elapsed time: 18.960000 msecs"
#4: "Elapsed time: 101.385000 msecs"
#5: "Elapsed time: 101.290000 msecs"

FIREFOX:

"OLD"
#1: "Elapsed time: 7.880000 msecs"
#2: "Elapsed time: 7.820000 msecs"
#3: "Elapsed time: 69.460000 msecs"
#4: "Elapsed time: 197.800000 msecs"
#5: "Elapsed time: 209.300000 msecs"
"NEW"
#1: "Elapsed time: 7.380000 msecs"
#2: "Elapsed time: 7.360000 msecs"
#3: "Elapsed time: 8.100000 msecs"
#4: "Elapsed time: 110.640000 msecs"
#5: "Elapsed time: 89.960000 msecs"
"OLD"
#1: "Elapsed time: 6.520000 msecs"
#2: "Elapsed time: 7.360000 msecs"
#3: "Elapsed time: 106.920000 msecs"
#4: "Elapsed time: 252.300000 msecs"
#5: "Elapsed time: 249.800000 msecs"
"NEW"
#1: "Elapsed time: 7.360000 msecs"
#2: "Elapsed time: 6.940000 msecs"
#3: "Elapsed time: 6.880000 msecs"
#4: "Elapsed time: 99.380000 msecs"
#5: "Elapsed time: 53.220000 msecs"
Comment by A. R [ 12/Jan/18 2:14 AM ]

Impact on truly (unchunked) lazy-seqs is neglibile (hard to measure due to garbage collection causing a lot of variance).

Comment by A. R [ 12/Jan/18 10:22 AM ]

New ticket for reduce for ChunkedCons: CLJS-2468

Comment by A. R [ 13/Jan/18 7:00 AM ]

Depends on CLJS-2469 for tests to pass. Patch is attached.





[CLJS-2469] ChunkedCons chunk-next returns empty seq instead of nil Created: 13/Jan/18  Updated: 13/Jan/18

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

Type: Defect Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-2469.patch    

 Description   

There is a bug in ChunkedCons. In Clojure ChunkedCons (correctly) always calls seq in chunked-next. In CLJS it's not done. But since ChunkedCons has to be lazy it pretty much always gets an (empty) lazy seq as the "more" part.

Bug:

(-> (map inc (vec (range 64)))
    seq
    chunk-next
    seq
    chunk-next)

Returns and empty sequence instead of nil. This hasn't surfaced yet since nothing calls chunk-next on a ChunkedCons (yet).



 Comments   
Comment by A. R [ 13/Jan/18 7:26 AM ]

Found another bug that surfaced: Current implementation calls -seq on more which could be nil and this would blow up. So the patch also make a tiny change to -next also just calling seq on more. Pretty straight forward.





[CLJS-1439] Add type annotations to goog-define defined vars Created: 01/Sep/15  Updated: 29/Oct/17

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

Type: Enhancement Priority: Trivial
Reporter: Martin Klepsch Assignee: David Nolen
Resolution: Unresolved Votes: 4
Labels: None

Attachments: Text File CLJS-1439.patch    
Patch: Code

 Description   

Currently it's still required to annotate goog-define constants with ^boolean to allow the Closure compiler to safely remove dead branches. The macro could emit the var with ^boolean metadata to make this unnecessary.

In general it would be nice to have similar annotations for the already defined constants like goog.DEBUG although I'm not sure how/if that's possible.



 Comments   
Comment by Thomas Heller [ 28/Oct/17 5:25 PM ]

Added patch sets the proper :tag for the defined symbol for goog-define.

Inferring a type from a JS expression is not that simple and should probably be done in a separate ticket.

Comment by Thomas Heller [ 28/Oct/17 5:30 PM ]

Patch included a change from another ticket, fixed to only include the intended change.





Generated at Tue Jan 16 09:55:31 CST 2018 using JIRA 4.4#649-r158309.