Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use text properties instead of overlays for ANSI coloring #1500

Merged
merged 1 commit into from
Jan 3, 2016

Conversation

rfkm
Copy link
Contributor

@rfkm rfkm commented Jan 1, 2016

TL;DR

This PR improves the performance of REPL buffers by using text properties instead of overlays for ANSI coloring.
Also, this fixes #1452 while removing the hacky workaround introduced by #1455.


I recently sent some PRs related to overlays in the REPL buffer,
but I've finally realized we should avoid using overlays for ANSI coloring in the first place.
It's simply because overlays are pretty slow. I improved the performance a bit in #1488, but it's not radical.

So, in this PR, I remove all code related to overlays and use text properties instead.

I measured the performance by calling cider-repl--emit-interactive-output repeatedly.
The complete benchmark code is here, and the version of my Emacs is GNU Emacs 24.4.1 (x86_64-apple-darwin14.0.0, NS apple-appkit-1343.14).

active

As this figure shows, the overlay version (current master) is getting slower and slower as the number of lines in the REPL buffer increases.

total

Total time the overlay version spent on inserting 1000 lines is about 5 seconds. As for the text property version, it's about 0.6 seconds.

If no window displays the REPL buffer, it is much faster in the both cases:

hidden

total_hidden


I keep my changes in #1488 as is because they are harmless, but I can remove them if it is necessary.

I also wonder whether I should remove my old changelog entries related to overlays.
Since this PR removes old PRs' changes, it looks a bit strange that these changelog entries appears in the same release.

P.S.

While I had not intended that, the PR #1455 seems to have improved the performance significantly ;)

active_old
(This is the worst case, that is, all outputs have a trailing overlay.)

@Malabarba
Copy link
Member

Makes sense to me. Overlays are indeed very slow in large numbers.
Thanks for the very thorough analysis. :-)

Do we have tests for this ansi coloring feature?

@bbatsov
Copy link
Member

bbatsov commented Jan 1, 2016

I also wonder whether I should remove my old changelog entries related to overlays.
Since this PR removes old PRs' changes, it looks a bit strange that these changelog entries appears in the same release.

Yeah, you should remove them.

Overall - impressive work. I was amazed by your comprehensive analysis of the problem!
Performance optimization work is critical, but we've always ignored it to some extent so far. Maybe you can help us with tickets like:

Do we have tests for this ansi coloring feature?

Pretty sure we don't. A few tests would be nice to have.

@Malabarba
Copy link
Member

Also, I forgot to mention, but the code is good to merge for me (though some tests would be nice).

@bbatsov
Copy link
Member

bbatsov commented Jan 3, 2016

Yeah, it's good to merge for sure - I just want us to update the changelog accordingly as well.

@rfkm rfkm force-pushed the use-text-prop-for-ansi branch from 332a2e6 to 1e7a874 Compare January 3, 2016 15:30
@rfkm
Copy link
Contributor Author

rfkm commented Jan 3, 2016

Sorry for the delay.
I've just pushed some changes.
Since this PR also fixes #1452, I kept the changelog entry about #1452 though I removed the mention of overlays.
Instead, I've added summary of this PR on top of this thread.
Also, I added a test for this.

Performance optimization work is critical, but we've always ignored it to some extent so far. Maybe you can help us with tickets like:

I've already noticed cider-repl--show-maximum-output could be slow under certain conditions, but I hope one of the causes has been already eliminated.
One of the causes I noticed is (recenter -1) that cider-repl--show-maximum-output calls could be pretty slow if there are many overlays.
Also, I've noticed eldoc could be very slow in a large REPL buffer. As for this, I might send a PR in the near future.
Anyway, I'll try to investigate their issues :)

bbatsov added a commit that referenced this pull request Jan 3, 2016
Use text properties instead of overlays for ANSI coloring
@bbatsov bbatsov merged commit e284271 into clojure-emacs:master Jan 3, 2016
@bbatsov
Copy link
Member

bbatsov commented Jan 3, 2016

Great! Thanks a lot! :-)

@Malabarba
Copy link
Member

Thanks indeed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ANSI coloring in REPL buffer is broken
3 participants