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

EnhancedTermTypedCharSupport: store queued chars in a deque #11418

Closed
codeofdusk opened this issue Jul 24, 2020 · 4 comments
Closed

EnhancedTermTypedCharSupport: store queued chars in a deque #11418

codeofdusk opened this issue Jul 24, 2020 · 4 comments

Comments

@codeofdusk
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Currently, the EnhancedTermTypedCharSupport class stores its queue of typed characters in a list. Per the Python docs:

Though list objects support similar operations, they are optimized for fast fixed-length operations and incur $\mathcal{O}(n)$ memory movement costs for pop(0) and insert(0, v) operations which change both the size and position of the underlying data representation.

Describe the solution you'd like

Use a deque instead. pop characters to be dispatched instead of iterating over the entire list and rebuilding it.

Describe alternatives you've considered

Keep the current implementation: the number of queued characters is small and time/space savings are theoretical.

@feerrenrut
Copy link
Contributor

Before accepting a change like this I'd like to see it actually measured under standard and extreme conditions. IE prove that it doesn't make things worse. I agree, this should perform better, but good to know for sure it does in the cases that this code encounters.

@Adriani90
Copy link
Collaborator

@codeofdusk are you still thinking implementing this makes sense? Or is the poerformance much better now?
If you think it makes sense, could you provide test results in extreme situations?

@codeofdusk
Copy link
Contributor Author

codeofdusk commented Nov 18, 2024

I have little confidence that a PR like this without obvious user benefit will be accepted, see #11484. It just doesn't meet the #11006 bar. If you'd like to provide benchmarks I'm happy to take this on.

@codeofdusk codeofdusk closed this as not planned Won't fix, can't repro, duplicate, stale Nov 18, 2024
@seanbudd
Copy link
Member

@codeofdusk that issue is closed, we welcome positive code refactors that increase maintainability

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 a pull request may close this issue.

4 participants