tonsky / Clojure-Sublimed

Clojure support for Sublime Text 4
MIT License
359 stars 22 forks source link

Measure time-taken in nREPL middleware #39

Closed jaihindhreddy closed 2 years ago

jaihindhreddy commented 2 years ago

Closes #13.

Still over-reports times by ~1ms, presumably because of default middleware.

One approach to make this better, could be by calling ls-middleware and swap-middleware to insert the timing middleware at the bottom of the stack. WDYT?

Also, there seem to be calls to update with value but without time_taken. In such cases, I'm falling back to client-measured time. What am I missing?

tonsky commented 2 years ago

Can you remove client-side measured time? I think if something doesn’t work out in middleware, well, not a big problem, you just won’t see the time.

As for ordering middlewares, I don’t think that worth the compexity. Let it fall where it falls and let’s hope all the other middlewares are not too expensive.

I think we have to make it execute inside wrap-print at least, because printing might become expensive. Can you add this dependency too?

jaihindhreddy commented 2 years ago

Can you remove client-side measured time? I think if something doesn’t work out in middleware, well, not a big problem, you just won’t see the time.

Makes sense. Done.

As for ordering middlewares, I don’t think that worth the compexity. Let it fall where it falls and let’s hope all the other middlewares are not too expensive.

Makes sense too.

I think we have to make it execute inside wrap-print at least, because printing might become expensive. Can you add this dependency too?

Done.

Evaluating (+ 1 2) with ./script/nrepl.py running on the same machine as Sublime shows me a time in the 0.8 ms to 1.09 ms range. I'm not quite satisfied with this measurement overhead, but can't think of any clean ways to reduce it. WDYT?

tonsky commented 2 years ago

Might be an overhead of creating a thread, waiting for it etc. I think it’s a great improvement anyways, thank you!