Fork me on GitHub
#cljs-dev
<
2017-06-12
>
tmulvaney10:06:14

Seeing as equiv-map is now only used my maps which implement IKVReduce, replacing the every? call with a reduce-kv gives a nice speed up of 3-5x. Would a patch for this be welcome?

thheller12:06:13

I’d say thats a definite yes 🙂

tmulvaney12:06:53

Cool, made a ticket.

dspiteself16:06:43

We use records as keys in a map. You made the -equiv check cheaper now we just need to reduce collisions https://dev.clojure.org/jira/browse/CLJS-1977

anmonteiro16:06:38

PSA: please remember to run self-host tests when developing patches 🙂

anmonteiro16:06:49

particularly when touching cljs/core.cljc

anmonteiro16:06:07

a common gotcha is needing to prefix some function calls with core/

anmonteiro16:06:23

otherwise self-host tests will fail

tmulvaney16:06:37

@anmonteiro thanks for the pointer. I'll try not to forget!

favila17:06:57

FYI @dnolen @tmulvaney every call of the private fn equiv-map is of the pattern (-equiv [coll other] (equiv-map coll other)) where coll has an IKVReduce impl. I don't think the fallback was necessary

favila18:06:27

also the fallback is bad

favila18:06:33

(every? (fn [xkv] (= (get y (first xkv) never-equiv)
                               (second v)))
                  x)

favila18:06:39

v is undefined

favila18:06:58

meant (second xkv)

dnolen18:06:10

@favila fallback is needed, we don't know who else implements maps

favila18:06:34

equiv-map is private, and all call sites are known

dnolen18:06:54

We don't enforce private

favila18:06:17

don't we warn?

dnolen18:06:44

I don't recall but equiv-map being private isn't important

dnolen18:06:53

It could just as well be public

favila18:06:18

ok, then we need a test that actually covers the fallback case

favila18:06:42

but I would consider any codebase suspect that called equiv-map from outside core

thheller18:06:45

pretty sure all record impls also use equiv-map

favila18:06:03

the docstring for equiv-map is also curious

favila18:06:36

says "assumes y is a map" but also calls (map? y) on it

thheller18:06:05

could maybe move the reduce-kv impl to PAM and PHM directly and leave equiv-map untouched

favila18:06:05

ObjMap, PAM, PHM, PTM are the equiv-map callers (for completeness)

dnolen18:06:34

I'm fine with equiv-map being a public helper the implementation isn't hiding anything meaningful

favila18:06:00

You are fine with it being a public helper but not fine with it being private?

favila18:06:02

FWIW crossclj doesn't know of any uses, and a search for equiv-map shows some clear copy-pasting.

favila18:06:17

I don't know how good crossclj is at finding a ref like that though

dnolen18:06:31

I just mean why hide a useful thing for would be implementer of map types

favila18:06:28

I agree, but if it is private now, might as well exploit its privateness to tune the impl. if it's gonna be public, it should be cleaned up a little

dnolen18:06:43

Agree with that

shaunlebron18:06:14

@dnolen parinfer users can’t contribute patches without creating large diffs. if it helps them and welcomes more newcomers for us, would you consider running a light linter on the repo after merging patches? (changes are small and it runs in ~0.5s)

npm install -g parlinter
parlinter --trim --write "{src,test}/**/*.{clj,cljs,cljc}"
https://github.com/shaunlebron/parlinter

dnolen18:06:20

@shaunlebron sorry not interested in that.

shaunlebron18:06:22

@dnolen: thanks for looking. let me know if something could’ve made it fit better

dnolen18:06:07

Things that create whitespace changed is just a non-starter

dnolen18:06:15

I'm also not convinced this will have a significant impact on newcomer contribution. But happy to hear more about why you think it will - lots of people mentioning this problem to you?

shaunlebron19:06:12

thanks for feedback, curious why whitespace is a non-starter (e.g. it respects two-space indentation that I think you prefer)

shaunlebron19:06:05

re: parinfer feedback - I don’t get a lot of it, but Cursive users have complained about this, and i’ve read threads where it is a non-starter not being able to collaborate with non-parinfer users

shaunlebron19:06:03

i designed parinfer to be a newcomer tool, so I think it’s kind of a blow to the whole purpose if they can’t collaborate with it

moxaj19:06:31

when, as a parinfer user, I open a file and the editor says 500 lines would have to be changed, I have to refuse and then the file's like a minefield

juhoteperi19:06:44

Just looking at the parlinter diff and there is one change that I'm not fan of, where some code is commented with ; and the end parentheses are moved to next line. Parlinter moves those parentheses to line before comment.

juhoteperi19:06:16

(ns repl.test
-  (:require [clojure.browser.repl :as repl]
+  (:require [clojure.browser.repl :as repl]))
             ;[clojure.reflect :as reflect]
-            ))

dnolen19:06:50

@shaunlebron whitespace changes are just noise, why do we want that?

dnolen19:06:17

It also means people who don't use parinfer needs to use it to submit patches

shaunlebron19:06:40

@dnolen: that’s actually not true

juhoteperi19:06:00

I don't see what would the noise matter, perhaps it'd make looking at blame a bit harder

shaunlebron19:06:02

whitespace changes are a one-time diff, future lint usage prevents whitespace changes after that

shaunlebron19:06:14

it’s the same with Prettier/refmt/gofmt in other langs

dnolen19:06:41

Only if you opt in

thheller19:06:52

I agree with Shaun here, I had to re-write some patches just because I accidentally pressed re-indent file in cursive

dnolen19:06:20

I don't find any of these whitespace linters conpelling

shaunlebron19:06:52

i find them very aggressive too

dnolen19:06:12

The only way this would be considered is it's not invasive for core devs and contributors alike

shaunlebron19:06:16

i deliberately designed this one to be a compromise of current styles while allowing people to opt-in to parinfer

dnolen19:06:27

It doesn't sound like that's possible to me

juhoteperi19:06:01

One time style fix is not very invasive? All the problems it fixes seem lime real style problems, bad indentation etc.

shaunlebron19:06:14

there’s a misunderstanding then, the workflow is the same as I understand it, except you run 0.5s command before committing, yeah?

juhoteperi19:06:17

Those problems shouldn't be created when using cursive/vim/emacs/parinfer

juhoteperi19:06:28

Not sure how the current problems have been created

dnolen19:06:29

"Bad indentation" assumes there's a formal style there is not

juhoteperi19:06:48

There is style that 90% of the code uses

dnolen19:06:10

@shaunlebron but everyone has to do that when they didn't before

shaunlebron19:06:15

@dnolen: true, but you are the gate keeper of patches, and maybe i could write you a script that merges in the patch and lints it

darwin19:06:23

@shaunlebron what about instructing people to solve the problem at git level, locally working on a dirty copy corrected by parlinter and then using some git tools to do interactive staging and create patch from relevant parts only

rgdelato19:06:28

in a recent experimental project, we run Prettier as a pre-commit hook, so individual developers don't have to manually run the formatter. something like that could be possible. though I definitely understand and appreciate the cautious approach

darwin19:06:40

I personally use gitup.app on mac

shaunlebron19:06:01

nobody else would have to care about running it

dnolen19:06:30

Whitespace commits is a non-starter

dnolen19:06:19

Pre-commit hooks sounds reasonable if you care about this and there was a style guide formal thing in place

dnolen19:06:38

But there isn't, indentation is wildly different across projects

dnolen19:06:35

I could be convinced if surveys start showing large amount of parinfer usage

shaunlebron19:06:58

chicken and egg I think, people won’t use it unless it’s collaborative to begin with

dnolen19:06:12

No chicken egg

dnolen19:06:23

If lots of beginners use it compelling

dnolen19:06:57

That's separate from changing clojurrscript dev process

juhoteperi19:06:48

What about not touching the dev process but doing one time fix for current "problems"?

juhoteperi19:06:06

I doubt there will be many new problems as people are using Cursive/Emacs etc.

dnolen19:06:27

You realize that will destroy git blame right?

shaunlebron19:06:13

ah I see, it would create a git blame wall

shaunlebron19:06:40

the github interface lets you go back now to previous change though very easily

shaunlebron19:06:52

that was a problem in the past

dnolen19:06:02

If you use GitHub for that I do not

juhoteperi19:06:04

It is a problem but I'm not sure how big, it touches 6% of the lines which I'd consider quite small change

dnolen19:06:39

Ok that's better that I would expect would want to see how bad it is

darwin19:06:49

well, git blame is not that strong argument IMO, you can walk blame history in git/github, it would create one extra hop

dnolen19:06:27

Disagree sorry

dnolen19:06:54

But if the damage isn't that bad - more relevant point

shaunlebron19:06:46

@juhoteperi were you looking at just git diff -w for the 6% number?

juhoteperi19:06:11

git diff --shortdiff

dnolen19:06:16

If someone wants to create a branch to show what will happen I will happily take a look

shaunlebron19:06:19

whitespace changes are large, mainly for indented multi-arity function bodies, which clojure-mode and clojure style guide format away anyway

juhoteperi19:06:36

2500 lines, against cloc of 40k

dnolen19:06:01

Number is less important than what

dnolen19:06:15

Huge changes to core or compiler or analyzer would stink

shaunlebron19:06:45

@dnolen: this is the big change:

;; bad
(defn foo
  "I have two arities."
  ([x]
     (foo x 1))
  ([x y]
     (+ x y)))

;; fixed
(defn foo
  "I have two arities."
  ([x]
   (foo x 1))
  ([x y]
   (+ x y)))

dnolen19:06:57

Yes I understand, but I would want to see the actual damage

shaunlebron19:06:18

i’ll fork and link the diff

shaunlebron19:06:22

dummy PR for parlinted cljs if we want to track comments: https://github.com/shaunlebron/clojurescript/pull/1/files

dnolen20:06:05

@shaunlebron yeah I don’t know it’s just a lot of changes and we have yet a very small amount of evidence that this will effect new contributor contribution

dnolen20:06:54

I’d like to get a Paredit/Parinfer question in the Clojure survey to understand impact

dnolen20:06:40

but based on last years survey if we assume most people using Atom are using Parinfer that’s quite below 10% of users

shaunlebron20:06:24

@dnolen: I understand. it’s of course hard to measure the effect on potential newcomers but it’s always what I’m looking at—talking to people outside the community about their impressions of lisp, etc. Maybe I can setup a survey outside the community for that

dnolen20:06:50

@shaunlebron that would be useful

dnolen20:06:02

there’s no need to wait for Clojure survey for this

shaunlebron20:06:58

sure, can I count on you to help share it?

shaunlebron20:06:15

a survey I mean

dnolen20:06:39

sure I’ll retweet but I think if you’re looking for a lot of data there’s the obvious places to reach out

dnolen20:06:51

#beginners, ClojureScript IRC, ClojureScript ML

shaunlebron20:06:35

thanks, I’ll be targeting the wider JS community though as well

dnolen20:06:05

outside of the Clojure community I don’t know how much help I can really offer

dnolen20:06:33

while I may command some “respect” in the JS community I don’t know that anybody actually listens to me 🙂

shaunlebron20:06:44

ha, most will at least hear it, some will listen

dnolen20:06:36

@shaunlebron another thing to consider is that I don’t necessarily know that beginners need to start with contributing to the ClojureScript compiler - I mean that’s cool and all but to be honest that’s not something we actually need

dnolen20:06:54

I have enough work cut out for me with the amount of contribution we currently have

dnolen20:06:01

it’s always scary to change hashing, pretty sure this is right but people should test it https://github.com/clojure/clojurescript/commit/15779cf5d2e1e55be6d015af92e4d0008eb46bb2

dspiteself20:06:45

great! thanks so much we will give it a look.

Alex Miller (Clojure team)20:06:16

@dnolen what could possibly go wrong? ;) when I screened this for Clojure I did a lot of bytecode reading (to verify the macros were right) and testing of hash values before and after for a variety of cases. Unfortunately, I didn’t keep any of that.

favila20:06:24

@alexmiller @dnolen I discovered that clojure's type name is hashed strangely

favila20:06:29

and not the same as this

favila20:06:40

whether that matters or not, shrug

dnolen21:06:16

I think not

favila21:06:33

clojure does (hash (symbol (str (namespace-munge *ns*) "." cname))

dnolen21:06:45

yes I checked the Clojure one

favila21:06:46

where namespace-munge is only - to _ replacement

favila21:06:44

(I was working on this the same time you committed, goal was hash same as clojure)

favila21:06:14

I also inlined caching-hash because of the one-arity function oddness

favila21:06:01

also, hash-coll, hash-imap hash-iset become unused after this, so deleted those too. (They belong to the pre-murmur days)

mfikes21:06:22

Plank was fine with the hash change

mfikes21:06:40

Re-Natal (Om Next on React Native on iOS) sample app was fine with the hash change

dnolen22:06:09

@mfikes cool thanks for the confirmations

dnolen22:06:18

still looking for more if anyone has them of course 🙂

dspiteself23:06:14

we had to upgrade a ways and are having issues with relative paths to closure libraries. hopefully I will get you confirmation tommorow

mfikes23:06:32

I'm excited about the prospect that we can ratchet up the test quality via generative testing: https://dev.clojure.org/jira/browse/CLJS-2082