cljfmt icon indicating copy to clipboard operation
cljfmt copied to clipboard

Refactor indenting

Open or opened this issue 2 years ago • 3 comments

This doubles the speed for multi file processing in a native image. By separating the identification of parent form-symbols from the rule matching and using map look-ups.

Related to #254.

or avatar May 07 '22 21:05 or

Commit messages of this repository should follow the seven rules of a great Git commit message, as mentioned in the project's contributing guidelines. It looks like there's a few issues with the commit messages in this pull request:

  • The commit message body has a line over 72 characters.

atomist[bot] avatar May 07 '22 21:05 atomist[bot]

Can you provide benchmarks? Does it improve the speed in Java as well?

In its current state, I'm afraid that the code isn't readable enough to be merged. Indexing the indentation rules in some way is a good idea, but we need to take care not to increase complexity or negatively impact maintainability. Can you give an overview of your design?

weavejester avatar May 07 '22 22:05 weavejester

The commit message includes concrete performance improvements, including the Java version. In short: it's not as big an impact, I'm thinking due to JIT, but still decent. It's hard to benchmark this with Java in particular, due to JIT and concurrency, but also because it's not easy to separate start-up time from processing time. For instance, the directory improvement given in the commit message probably underestimates the real impact substantially considering the measured times for a single file, which implies a ~2s start-up time.

I did a bunch of tests in Java and GraalVM with https://github.com/Tyruiop/needle and https://github.com/clojure-goes-fast/clj-async-profiler, focusing on number of calls and unnecessary work duplication.

The commit message also outlines the design a bit. I'm assuming you looked mostly at the diff yesternight, so let me know if the commit message already helps, otherwise I can write up some more details.

I think it already simplifies some aspects of the design, like indenter-fn and the main flow starting at custom-indent. But build-indent-rule-map and applicable-indent-rules definitely are a bit unwieldy. That's where most of the complexity lives, but the good news is that they also encapsulate the look-up logic.

or avatar May 08 '22 11:05 or