I did wrap the test suite in clojure.test today

and added some of the tests that occur in parinfer.js

Cool, I’ll make it so that lein test works, and then we can remove the main

ok

I was going to add a really rudimentary speed tests

the same speed test as parinfer.py; just to see how it compares there

basically parsing cljs.core

but I fell asleep in my hammock 😉

Hehe

Of course, I was doing this against my fork, and now I have to find the magic incantation to make git use the original repo

git remote set-url ?

Here’s hoping

@chrisoakman: Ok, this seems to work. I haven’t done anything to the code, but I’ve gradleised the main project (now under parinfer/ in the repo) and the tests now run using lein test and I’ve removed the main function.

ok

I’ll push this now, one sec.

@chrisoakman: Done, take a look.

I haven’t updated the README, actually, I’ll do that shortly.

Under /paredit, do ./gradlew install, then under /paredit-test do lein test

ok - I'll test that out

doing the speed thing right now

I’m going to check the actual code after some lunch.

I'm seeing around ~600ms for indent and paren mode on cljs.core

if these timestamps are to be trusted

which is slower than Python and JS

which is a bummer, if that's true

there is something about how the JVM "warms up" though? and could effect the time if it had been running for a long time

I don't know

@chrisoakman: How are you testing that?

https://gist.github.com/oakmac/e6749a63565249e296d6

so very rudimentary

Yeah, that won’t do it at all.

You really need to use criterium.

this is crazy...

you know my SENTINEL_NULL hack?

out of curiosity I made that same change to the JS version

seeing noticeable performance improvement because of it

gotta make sure this isn't a fluke...

I’m replacing that with actual nulls at the moment

In the JVM version.

we should probably get a performance testing harness up

@chrisoakman: I see what you mean about the null checking being difficult. It’s because the result fields are vars, so you don’t get a lot of the nice smart casting, because multiple threads might be modifying it concurrently.

ok

man - this is nice

you know about the JVM

and I'm very familiar with parinfer implementation algorithm

one big question I had was: is it faster to use Java Objects or a HashMap?

for result

just tried the SENTINEL_NULL hack in Python: no speed improvement

if anything it might be a touch slower

this must be a JS engine optimization thing

under the hood

tell me I am not crazy:

https://gist.github.com/oakmac/ede280269cfd9660f051

https://gist.github.com/oakmac/b4200f2cf98f856af32b

but that is a noticeable perf improvement

https://github.com/shaunlebron/parinfer/pull/88

That’s bizarre

just added a table of the result averages

it's almost hard to believe how much the difference is on the long file

@cfleming: we gotta set up a performance harness for the JVM version

well

actually - maybe more important to get it in Cursive first?

@chrisoakman: Ok, I’ll do that next.

No, I think we should perf test it.

I have some more changes to check in.

I can't even believe that JS difference; I'm double-checking my spreadsheet now

Yeah, that’s bizarre alright, but who knows what the JIT does with it?

I guess v8 optimizations are not to be under-estimated

Absolutely not.

I’m planning to create a ParenTrail object to store in result. I believe that can contain all non-nullable vars, and then MutableResult can just have a nullable ParenTrail ref.

That will avoid a bunch of null checks and casts I had to do to make Kotlin happy.

ok

I have to go shortly, but the next thing I’ll do is set up a test harness for the perf testing.

you see I decided to just "add" the parenTrail object to the result?

ok

Actually, I haven’t compared that closely to the JS yet, I will do.

in the JS and python version "parenTrail" is an object / dict

I have some changes I’m about to push, all tests still pass.

kk

basically I didn't want to create another object definition, so I rolled it into the MutableResult object

but it would be closer in spirit to the JS version if there were a parenTrail object

Oh, that’s basically what I’m planning to do here - it’ll work better since then there’s just one check - do we have a paren trail or not, and not a bunch of null checks for each of the individual vars.

Ok.

gotcha

are objects faster than hashmaps?

that was my other big question

MutableResult could just be a HashMap

Yes, probably.

Again, have to test it, but almost certainly yes.

Just pushed my changes.

You’ll see a bunch of as Int casts, they can go with the ParenTrail change I think.

Ok, gotta go, talk to you later.

later

let's make sure we keep the README up-to-date

I'll add the new gradle instructions

Great, thanks.

@chrisoakman: Here are the perf results using Criterium:

~/d/p/parinfer-test (master)> lein with-profile bench run
Evaluation count : 1380 in 60 samples of 23 calls.
             Execution time mean : 45.121138 ms
    Execution time std-deviation : 446.875947 µs
   Execution time lower quantile : 44.435676 ms ( 2.5%)
   Execution time upper quantile : 46.004253 ms (97.5%)
                   Overhead used : 1.672151 ns
Evaluation count : 1380 in 60 samples of 23 calls.
             Execution time mean : 45.253843 ms
    Execution time std-deviation : 1.039081 ms
   Execution time lower quantile : 44.084716 ms ( 2.5%)
   Execution time upper quantile : 47.587892 ms (97.5%)
                   Overhead used : 1.672151 ns

Found 3 outliers in 60 samples (5.0000 %)
	low-severe	 3 (5.0000 %)
 Variance from outliers : 10.9943 % Variance is moderately inflated by outliers

~45ms

I saw there’s some spots that I can optimise there, too, I’ll do that now.

Those two results are indent mode and paren mode on really_long_file