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

Make tab extension lazy #684

Merged
merged 1 commit into from
Jan 15, 2025
Merged

Make tab extension lazy #684

merged 1 commit into from
Jan 15, 2025

Conversation

jaheba
Copy link
Contributor

@jaheba jaheba commented Jan 13, 2025

Running the snippet of #683 a big chunk of the time is spent in allocating a new string in TabExpandedString. Delaying the expanding of tabs reduces the runtime of ~100ms to ~60ms.

The idea is simple: Rather than replacing \t with spaces immediately, this is done once expanded is actually called.

@chris-laplante
Copy link
Collaborator

Makes sense to me, thanks! Just kicked of CI to verify.

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that looks like a nice win!

Also, would be nice if you can squash your commits.

@@ -346,21 +347,20 @@ pub(crate) enum TabExpandedString {
NoTabs(Cow<'static, str>),
WithTabs {
original: Cow<'static, str>,
expanded: String,
expanded: OnceCell<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use Option instead of OnceCell for this? Seems simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think so: We don't have mut access in expanded. AFAIK, that's essentially what OnceCell is there for.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough!

@jaheba jaheba force-pushed the lazy-tab-expansion branch from e8de6a3 to 803b013 Compare January 15, 2025 15:50
@djc djc merged commit fff1218 into console-rs:main Jan 15, 2025
10 checks passed
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.

3 participants