jscheid / prettier.el

Prettier code formatting for Emacs.
GNU General Public License v3.0
168 stars 12 forks source link

Batch Prettier changes #66

Closed jscheid closed 2 years ago

jscheid commented 3 years ago

When Prettier formats a file, especially when it formats the file for the first time, doing so may result in a large number of small changes to the buffer.

The improvement in this PR uses heuristics and the combine-change-calls macro in an attempt to reduce the number of hook calls that may happen in this scenario.

The relevant Emacs documentation says about this that

[the combine-change-calls] macro is useful when a function makes a possibly large number of repetitive changes to the buffer, and the change hooks would otherwise take a long time to run, were they to be run for each individual buffer modification. Emacs itself uses this macro, for example, in the commands comment-region and uncomment-region.

And in the related documentation for combine-after-change-calls:

If a program makes several text changes in the same area of the buffer, using the macro combine-after-change-calls around that part of the program can make it run considerably faster when after-change hooks are in use.

Sounds great, doesn't it? But I'm ambivalent about the value of this improvement. I've implemented it because I had made some (unscientific) observations of lackluster performance that I had attributed to a large number of small changes, and had hoped it would help in those situations. However, after building this change (and verifying that it works as intended, by logging the number of batched changes applied) I have yet to find evidence that it does.

At the same time, I also haven't noticed any slowdowns resulting from this change, and so am slightly leaning in favor of merging it as a preemptive guard against large latency in degenerate cases, now that it's already implemented. It's also possible that the only reason I can't notice any improvements is due to my particular Emacs setup (which might not make heavy use of change hooks).

There is no rush however. I will leave this sitting here for a while to give people a chance to play with it. As always, you can follow the CI checks on this PR to find the artifact that you can then install using package-install-file (after unzipping it.)

If you do so, it would be great if you could report here whether this change improves things, makes things worse, or doesn't seem to make any difference at all. It would help me decide whether to go ahead with merging it eventually, or whether to treat it as a failed experiment and discard it.

I would also appreciate if you could look over the code to see if you can spot any errors in the implementation that would render it less effective.