<< Back to previous view

[CLJS-846] Preserve namespace metada Created: 27/Aug/14  Updated: 28/Aug/14  Resolved: 28/Aug/14

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

Type: Enhancement Priority: Major
Reporter: Joel Holdbrooks Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 846.patch    

 Description   

Namespace metadata is currently discarded and not available on cljs-ns/ns. Preserving this metadata is useful for some macros.



 Comments   
Comment by David Nolen [ 28/Aug/14 9:53 AM ]

fixed https://github.com/clojure/clojurescript/commit/edd35fa206d8285b2d671264347a1f0024631b13





[CLJS-839] Keyword hashing mobile safari (ios7) w/ advanced compilation Created: 11/Aug/14  Updated: 27/Aug/14  Resolved: 27/Aug/14

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

Type: Defect Priority: Major
Reporter: Christian Karlsen Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File cljs-839.patch    

 Description   

I've been debugging some really funky behavior from mobile safari and found the root cause to be keyword hashing.

Version >= 2261 seems to be affected. Please see https://github.com/ckarlsen84/cljs-keyword-hashtest for a quick and dirty minimal test app (`lein ring server`).

Output from some of my runs: https://gist.github.com/ckarlsen84/4c16684c1f9e5ce26711

It seems to only happen with advanced compilation and mobile safari.



 Comments   
Comment by Thomas Heller [ 12/Aug/14 4:34 PM ]

Duplicate of CLJS-830. Nice to have a better example, was way too hard to reproduce in my app.

Comment by David Nolen [ 12/Aug/14 5:03 PM ]

This makes me think it's an operator precedence issue. But who knows, will look into it. Oh can we get details on the hardware? I was actually unable to repro this at least in Thomas Heller's case on an iPhone 5S running iOS 7.

Thanks for the minimal case!

Comment by Christian Karlsen [ 12/Aug/14 6:38 PM ]

I ran the tests with an iPhone4 (MC603KN/A).

Comment by David Nolen [ 18/Aug/14 9:54 AM ]

Christian, am I correct in interpreting the results as actually hashing incorrectly only after several initial runs?

Comment by David Nolen [ 21/Aug/14 5:37 PM ]

In order to investigate, this ticket needs more information. Specifically it would be useful to know if native Math.imul or the shim is being used and to confirm whether the behavior occurs only after multiple runs.

Comment by Christian Karlsen [ 22/Aug/14 2:15 AM ]

Only happens after multiple runs (usually after 10-12 runs iirc). I'm travelling atm but I'll do some more tests during the weekend.

Easiest way to test if i'm using the shim or not?

Christian

Comment by David Nolen [ 22/Aug/14 7:15 AM ]

We def a imul fn based on the presence and correctness of js/Math.imul. Just do imul.toString() or something like that to figure out what you have.

The multiple runs thing makes me suspect the bug only manifests when JavaScriptCore JIT (or more efficient interpreters) kick in. This would also explain why people can't repro with the debugger attached.

Comment by David Nolen [ 26/Aug/14 12:03 PM ]

I've attached a patch (which I am unable to test since I don't have a suitable device), it would be great if anyone encountering this issue could test the patch, thanks.

Comment by Thomas Heller [ 26/Aug/14 2:40 PM ]

I did not try the patch, but I used the imul.js file and just included it in my HTML before ANY other generated javascript. Which should have the same effect as the patch.

It seems to fix the issue, but I'm too tired to do a complete check. Will do a complete check tommorrow. Looks promising though.

Comment by Thomas Heller [ 27/Aug/14 3:14 AM ]

I had the fix deployed over night, but it seems clients are still sending me maps with duplicate keys which means it doesn't work.

Will investigate later.

Comment by Kris Jenkins [ 27/Aug/14 4:12 AM ]

I've tried this test-case on an iPhone 5 (MD299B/A), and it breaks as expected. With the patch against 4b81710, it works.

Comment by Kris Jenkins [ 27/Aug/14 5:06 AM ]

Yep, this patch also fixes the bug we were seeing on our production builds under 2311. Woo-hoo!

Comment by David Nolen [ 27/Aug/14 8:30 AM ]

Kris thanks for the feedback. Looking forward to hearing if Thomas Heller sees positive results after further investigation.

Comment by Thomas Heller [ 27/Aug/14 9:05 AM ]

So far I have not been able to reproduce the bug when the fix is applied.

I forgot to include the fix on some pages which explains why I was still getting invalid maps from Safari clients.

Will keep an eye on the log but so far it seems to be resolved.

Comment by David Nolen [ 27/Aug/14 9:37 AM ]

Thanks everyone for the help on this one, fixed in master and I cut a new release 0.0-2322 https://github.com/clojure/clojurescript/commit/3c0b8e71e09a971a07e3eef70bd74df6f1104d92

Comment by Kris Jenkins [ 27/Aug/14 9:44 AM ]

Fantastic! Thanks David.





[CLJS-844] Optimize js->clj by switching to transducers Created: 22/Aug/14  Updated: 26/Aug/14

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

Type: Enhancement Priority: Trivial
Reporter: Darrick Wiebe Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File use-transducers-in-js-to-clj.patch     Text File use-transducers-in-js-to-clj.patch    

 Description   

This simple patch just switches a couple of lines to use transducers. I made this change after finding that using transducers for these types of mappings is significantly faster in (at least in Chrome).

I've checked that this code is being actually tested and both pass under V8 and JavaScriptCore.



 Comments   
Comment by David Nolen [ 23/Aug/14 1:19 PM ]

Did you any benchmarks on other JS engines?

Comment by Darrick Wiebe [ 23/Aug/14 2:14 PM ]

No. My main concern was to check if there was any startup overhead that might offset the basic efficiency improvements for processing small collections. Using into with a transducer turned out to be faster in every scenario, significantly in most.

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

It would be nice to see a jsperf of before and after on this ticket. Thanks.

Comment by Darrick Wiebe [ 23/Aug/14 2:23 PM ]

Is there a existing one that I can work from?

Comment by David Nolen [ 23/Aug/14 2:35 PM ]

There is not. I generally just make a simple project, create an advanced compiled artifact and copy and paste it into jsperf.

Comment by Darrick Wiebe [ 24/Aug/14 7:24 PM ]

Turns out reducing into a transient is considerably better than using a transducer (which was itself a little faster) for this.

http://jsperf.com/js-clj-transducer-test

The code is at:

https://gist.github.com/pangloss/591d77231fda460c2fbe

Let me know if you want me to prepare an updated patch.

Comment by David Nolen [ 25/Aug/14 7:34 AM ]

Thanks for putting this together. Yes please provide an updated patch.

Comment by Darrick Wiebe [ 26/Aug/14 11:19 AM ]

Not sure whether the convention is to comment that I've uploaded a new patch. Regardless, I uploaded it yesterday.





[CLJS-845] incorrect behavior of sequence when given multiple collections Created: 25/Aug/14  Updated: 25/Aug/14  Resolved: 25/Aug/14

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

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


 Description   
(def xf (map #(+ %1 %2)))
(sequence xf [0 0] [1 2])

Results in:

TypeError: Cannot find function hasNext in object


 Comments   
Comment by David Nolen [ 25/Aug/14 12:00 PM ]

fixed https://github.com/clojure/clojurescript/commit/4b81710971a1a662fdeb2db525328c3443a72545





[CLJS-843] Compiling with :target :nodejs, resulting code loses CL args Created: 22/Aug/14  Updated: 23/Aug/14  Resolved: 23/Aug/14

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

Type: Defect Priority: Minor
Reporter: Peter Schwarz Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: nodejs
Environment:

Mac OS X 10.9.4, JDK 1.7.0_05, CLJS 0.0.2311



 Description   

Compiling with {:optimizations :advanced :target :nodejs} command line arguments are lost.

Using the following example code:

hello_cljs/core.cljs
(ns hello-cljs.core
    (:require [cljs.nodejs :as nodejs]
              [clojure.string :as s]))
 
(defn say-hello 
  [& args]
  (str (s/join " " (remove nil? (conj args "Hello")))))

(defn -main [ & args ]
  (println "Running...")
  (println (apply say-hello args)))
 
(nodejs/enable-util-print!)
(set! *main-cli-fn* -main)

with :simple optimizations:

> node hello.js me
Running...
Hello me

with :advanced optizations:

> node hello.js me
Running...
Hello

Expected behaviour should be that the results are equivalent.



 Comments   
Comment by David Nolen [ 23/Aug/14 1:32 PM ]

You need to supply an externs file for Node.js.

Comment by Peter Schwarz [ 23/Aug/14 4:24 PM ]

It would probably help to have this mentioned in the documentation, specifically in the node case, since all of the documented examples w.r.t. Node.js use :optimization :advanced.

The need for an externs file would also explain why the nodels.cljs sample doesn't work, as well.

Comment by David Nolen [ 23/Aug/14 4:34 PM ]

I've updated the ClojureScript Quick Start to point out there is no need for advanced optimizations under the Node.js target as well as explaining what is needed if for some reason it is actually necessary.

Comment by Peter Schwarz [ 23/Aug/14 4:43 PM ]

Great. Thanks!





Generated at Fri Aug 29 11:16:08 CDT 2014 using JIRA 4.4#649-r158309.