Skip to content

Conversation

@xizheyin
Copy link
Member

@xizheyin xizheyin commented Aug 9, 2025

In the original iterative approach, we only inserted one element at a time, which meant that the entire string had to be moved each time, leading to the complexity being O(m*n). We could use splice to move it only once, whose complexity is O(m+n). I have conducted preliminary testing on the local UI test and found some improvement (but I am not certain).

r? compiler

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 9, 2025
@Kivooeo

This comment was marked as resolved.

@fee1-dead
Copy link
Member

this makes it super hard to understand. I would have preferred us just collecting from a chain

@fee1-dead
Copy link
Member

prepending by converting first into a vecdeque and converting it back is probably as efficient

@xizheyin
Copy link
Member Author

xizheyin commented Aug 9, 2025

What about

row.resize(n + k, StyledChar::SPACE);
row.copy_within(0..n, k);
for (i, ch) in string.chars().enumerate() {
    row[i] = StyledChar::new(ch, style);
}

It can move right the elements in place, with the similar performance and better readablity. @fee1-dead

@fee1-dead
Copy link
Member

This code path is only exercised when there is a compile error so its not prioritized. Why should we insist on this buffer having everything done in-place?

If prepend perf is so important, why aren't we switching to use a VecDeque?

@xizheyin
Copy link
Member Author

xizheyin commented Aug 9, 2025

If prepend perf is so important, why aren't we switching to use a VecDeque?

I'll try this method.

This code path is only exercised when there is a compile error so its not prioritized.

I see many places where it is called in the diagnostic information (but I'm not sure if this is a hot path), and I need to do enough experiments to check. In my preliminary testing, it seems to speed up UI testing (though I'm not entirely sure).

@fee1-dead
Copy link
Member

that's not how i wanted you to use vecdeque

@xizheyin
Copy link
Member Author

xizheyin commented Aug 9, 2025

Could you tell me how to use it better?

@xizheyin
Copy link
Member Author

After investigation, I figure out that more than half of the callsite use short string arg whose length is 1-5, so the performance effect may not be so important. Let's keep it as it is. At least it is easy to read.

@xizheyin xizheyin closed this Aug 10, 2025
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2025
@xizheyin xizheyin deleted the prepend branch August 10, 2025 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants