Fork me on GitHub
#cljs-dev
<
2016-06-08
>
pat00:06:52

so on JVM nil is seqable

pat00:06:08

should I extend it here? I imagine this may have implications

dnolen01:06:45

@pat no you don’t need to extend it

dnolen01:06:50

you can call seq on nil ClojureScript as well

dnolen01:06:06

all the stuff around sequable isn’t important for ClojureScript

dnolen01:06:09

we already have it

mfikes01:06:32

A data point: if you do it, seqable? returns true, (-seq nil) works, and the unit tests pass. (Not offering opinion—just some data about it.)

dnolen01:06:30

yeah we’re not going to do that

dnolen01:06:37

extend-type on nil is too slow

dnolen01:06:59

if (seqable? nil) is false, just add a conditional in seqable? to check nil

dnolen01:06:25

operations on nil used to be very bad

dnolen01:06:34

we extend-type’d it everywhere

dnolen01:06:37

I ripped that stuff out years ago

mfikes01:06:37

Yeah, it would probably slow down a lot of things for no significant value

pat01:06:28

@dnolen ok. I'm noticing too that the simple/qualified predicates for keywords return nil instead of false. Should i wrap the namespace check with boolean (inside the and)?

mfikes01:06:01

@pat we generally need predicates to return true or false so they can be hinted ^boolean so inference can lead to unchecked ifs

mfikes01:06:41

@pat looks like you signed the CA, perhaps twice unless your name is very common

pat01:06:34

@mfikes yes talked to alex about this

pat01:06:52

Is different than as written in the jvm patch is all

mfikes01:06:05

Ooh. How do we handle seqable? on strings. Are there other special cases?

mfikes01:06:15

I suppose the answer to this question is all the cases in seq (`array?` etc.)

mfikes02:06:42

@pat I can’t get simple-keyword? or qualified-keyword? to return nil. (I’m only getting true or false.

pat02:06:26

i may have mutilated it, let me check. it's been a long day,

mfikes02:06:00

@pat With respect to boolean? there is a patch in http://dev.clojure.org/jira/browse/CLJS-1241 which might be useful

pat02:06:53

@mfikes clojure and should return nil or false, which is what im getting

pat02:06:51

(namespace :foo) ;=> nil

mfikes02:06:07

(defn simple-keyword?
   "Return true if x is a keyword without a namespace"
   {:added "1.9"}
   [x] (and (keyword? x) (nil? (namespace x))))

mfikes02:06:11

That fn ^?

pat02:06:20

i'm looking at qualified

mfikes02:06:01

Ahh, you are right

pat02:06:53

the predicate table in the jvm patch lacks the new symbol+kw predicates, i just added them and noticed

pat02:06:15

I added boolean as (or (cljs.core/true? x) (cljs.core/false? x)), should I pull it out?

pat02:06:23

boolean? that us

pat02:06:27

is* geez

mfikes02:06:46

I think David was OK with a unified patch containing all of the new stuff

mfikes02:06:03

Just suggesting that other ticket as an inspiration for an impl

pat02:06:06

ok I figured

dnolen02:06:01

@pat it’s fine to chat about the details with Alex tomorrow

pat02:06:08

I will snatch out the test at least

dnolen02:06:16

though I don’t see why it’s important that the predicate return boolean

pat02:06:23

I emailed him about it awhile ago, will send him a reminder

dnolen02:06:26

it’s fine that it returns nil instead of false

dnolen02:06:34

it’s means the ^boolean thing is a lie, but a harmless one

mfikes02:06:52

Gotta love ClojureScript’s code generation. This is much more optimal than it looks:

(defn qualified-keyword?
   [x] (and (keyword? x) (not (nil? (namespace x)))))

dnolen02:06:02

oh yeah 🙂

mfikes02:06:06

David, if you hint ^boolean and return nil that’s one example we can be lax on right. It is 0 and ”” which cause the grief...

dnolen02:06:28

as long as we know a fn won’t return those values, lying is OK

mfikes02:06:41

Cool, I actually hadn’t considered that aspect. Makes sense.

pat03:06:31

I put the easy stuff in a patch

pat03:06:38

the predicate table could be richer

mfikes03:06:11

I suppose with CLJS-1672, a decision could be taken on whether macro versions of some of the new predicates are useful, or maybe that’s overkill for now.

darwin15:06:55

just playing with demunging javascript indetifiers back to clojurescript names via demunge, it works pretty well except for when user decides to use dollars in cljs names. Dollars are not forbidden and they don’t get any special treatment in CHAR_MAP but do get special treatment on the way out in demunge-str because of internal cljs usage of dollars to encode namespace separators and js-reserved? stuff

darwin15:06:46

I believe user-specified dollars should have their place in CHAR_MAP (easy solution) or cljs should not use dollars for encoding anything (difficult solution)

dnolen16:06:15

@darwin I don’t $ is common for anything except compiler stuff

darwin16:06:29

well, I agree that people don’t use $ in their names (I discovered by accident by trying “crazy” names), but still for correctness I think demunge should be inverse function of munge

darwin16:06:49

I think adding $ into CHAR_MAP should not break anything important

darwin16:06:09

we will just generate _DOLLAR_ for user-specified dollars in names

dnolen16:06:36

you should try it

dnolen16:06:41

it’s not really clear to me that things won’t break

darwin16:06:32

btw. this will be a part of major new feature in cljs-devtools, I will demunge fn names and args when printing to console, and also part of "Beautification of function names” in Dirac, so I quite care about this to work correctly, because I will be on the receiving end of reports when it won’t work as expected 🙂

darwin16:06:51

I will create a JIRA ticked, with patch and some tests

darwin16:06:55

and we will see, I think this could break someone who would rely on dollars kept intact in munged names or copy&pasted CHAR_MAP into their code, which should be easy to fix if someone really did something like this

dnolen16:06:01

there usability things to think about here to for people who aren’t using cljs-devtools

dnolen16:06:16

all top level fns get munged names

dnolen16:06:24

like cljs$core$first

dnolen16:06:42

this works consistently across many JS target in provided better stack traces, better profiling dumps etc.

dnolen16:06:55

priotizing Chrome is non-starter

darwin16:06:03

I see, but that won’t change, the change will affect only names which will contain dollars in user code

dnolen16:06:25

right so as long as that’s the case - then not a problem

dnolen16:06:29

@darwin which is to say, patch welcome! 🙂

darwin16:06:40

working on it 🙂

darwin17:06:14

it will take me more time to figure it out, didn’t realize that cljs compiler at some point relies on clojure munge, so touching just cljs implementation is not enough

jr17:06:49

@dnolen: Here is an example of a serialized Var in older clojurescript analysis cache. Where schema-with-name is a var imported from another namespace https://gist.github.com/JacobNinja/f663ae6dcef3f2e83478880d07bf09fe

jr17:06:26

no #object tags just maps of :name :file :line :column

darwin17:06:33

also there may be code like this[1], which relies on fact that munge does not touch dollars: https://github.com/clojure/clojurescript/blob/c3899acf797eb6779c53b313f5606c5018360b83/src/main/clojure/cljs/compiler.cljc#L73 I will have to go carefully through all munge usages and make sure internal dollars are applied after munge, still no big deal, can be done

darwin17:06:13

=> (str (string/replace (munge (str ns)) "." "$") "$" (munge scoped-name)))))

dnolen18:06:27

@jr did you try master?

dnolen18:06:41

if that’s what your cache looks like then there is no issue

jr18:06:06

sweet - I’ll check it out and let you know

rohit18:06:33

shameless self promotion: i just blogged about performance aspects of using protocols in ClojureScript - http://blog.ducky.io/clojurescript/2016/06/08/more-defprotocol/

rohit18:06:36

comments are welcome

dnolen18:06:48

@rohit the conclusion somewhat inaccurate, nothing to do with protocols everything to do with apply

rohit18:06:32

@dnolen: thanks for that.

rohit18:06:24

@dnolen: i thought a contributing fact was that is was copying over arguments into a new array and using a switch the length of that array

rohit18:06:35

but i didn’t benchmark that

dnolen18:06:37

that cost isn’t really worth talking about

dnolen18:06:45

it’s costs something, but it’s incredibly small

rohit18:06:50

cool. thanks!

rohit18:06:37

@dnolen: i’ve been meaning to ask you: does it make sense to have a micro benchmarking suite for clojurescript so that those benchmarks can be observed over time

rohit18:06:12

obviously modern js engines are really fast

rohit18:06:35

and these micro benchmarks aren’t representative at all

rohit18:06:44

so i had mixed feelings about them

dnolen18:06:50

@rohit we have microbenchmarks in the repo already

dnolen18:06:06

it’s low hanging fruit to put together some code so it’s easier to see somewhere

rohit18:06:42

@dnolen: agreed. cool. i’ve been meaning to have a crack at that low hanging fruit for some time now.

rohit18:06:49

@dnolen: blog post fixed. thank you!

pat20:06:02

is the right test for 'nat-long' to check for zero or positive?

rohit20:06:29

@pat looks that way. code says its a long? and not neg?

rohit20:06:00

(not (neg? x)) <=> (or (pos? x) (zero? x))

dnolen20:06:32

@pat just write the appropriate ops for goog.math.Long

pat20:06:09

yes I'm just unfamiliar with whatever a natlong is

pat20:06:21

i guess it just considers 0

pat20:06:41

im there

pat20:06:43

im a biologist excuse my ignorance here 😕

dnolen20:06:12

in this case

dnolen20:06:20

(not (.isNegative n)) works

pat20:06:39

yes I have that for neg-long?

dnolen20:06:01

@pat is there something you are missing?

dnolen20:06:11

.isNegative and .isZero should have you covered

rohit20:06:20

@pat: so natural numbers are counting numbers - 0, 1, 2, 3 etc

pat20:06:36

ok yes just verifying the difference between positive check and nat check

pat20:06:21

@dnolen should I change the long fn & the integer? predicate to use goog.math.Long

dnolen20:06:39

@pat not in this patch no

dnolen20:06:03

probably not ever, since changing those are likely to cause needless breakage

dnolen20:06:27

the idea here is that people may communicate longs for various reason and we want to support that

dnolen20:06:33

not about adding longs for doing math stuff

pat20:06:56

just noting that there will be a conflict between using the long fn and the new long? predicate

dnolen20:06:56

i.e. you write a schema for an endpoint that will give you a long

dnolen20:06:06

@pat it’s not an important one

dnolen20:06:16

long is a coercion

dnolen20:06:40

but we don’t have long in JavaScript for math

pat20:06:58

yeah well thats why I ignored this in the first place

dnolen20:06:06

anyways whatever we decide, not in scope for this patch

darwin22:06:45

unfortunately my affair with munge turned out to be much more complicated, cljs.compiler uses munge quite deliberately at many places and can pass partially munged names, concatenate them with cljs$core$ specials, and then pass them down the pipe where they can get munged again when finally emitted. I don’t see an easy strategy how to implement my changes locally without touching a lot of code assuming dollars are not munged.

darwin22:06:40

I could rename all cljs internal dollars to some special unicode character and turn them into normal dollars at the emit level (where normal dollars were already munged into _DOLLAR_). But that smells like a hack and I doubt it would be accepted.

danielcompton23:06:03

Just to add to this, https://clojurians.slack.com/archives/cljs-dev/p1465401175000206 $ is used in functions with multiple arities

darwin23:06:32

yes, dollars are used at many places internally, I believe there is no easy way to distinguish dollars coming from user and dollars added in later compilation stages when munge method gets eventually called

darwin23:06:27

after my explorations today I think demunge-str is too aggressive, it replaces all dollars with “/“: https://github.com/clojure/clojurescript/blob/463de005b81d4a00951188e8b8d38a61d684c18e/src/main/cljs/cljs/core.cljs#L10350

danielcompton23:06:39

Dollars in functions from users is pretty unlikely, I don’t think I’ve ever seen one

darwin23:06:04

yes, I agree

darwin23:06:22

I was just curious if this can be fixed, didn’t expect it to be such PITA

danielcompton23:06:34

$ in CHARMAP seems like a good solution regardless though

darwin23:06:46

does not work, unfortunately

darwin23:06:38

munge which eventually uses CHAR_MAP can be often called on names constructed by cljs compiler containing internal dollars

darwin23:06:56

but munge has no chance to distinguish between them at that point

darwin23:06:21

I decided to go another route, I will implement my own version of demunge with heuristics which dollars are likely coming from cljs internals

darwin23:06:16

I have to do with namespaces now anyways, namespace separator is unfortunatelly dollar again, so I cannot tell which part of munged name is really a namespace

darwin23:06:38

I have to test goog datastructures if given part is existing namespace

darwin23:06:51

and take the longest one

darwin23:06:44

AFAIK there is no way how to tell from name alone, because user dollars and cljs internal dollars can complicate the resolution