Skip to content

Implement nth for EscapeUnicode #33932

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

Implement nth for EscapeUnicode #33932

wants to merge 4 commits into from

Conversation

ranma42
Copy link
Contributor

@ranma42 ranma42 commented May 28, 2016

and cleanup the code for nth for EscapeDefault by sharing the stepping logic between nth and next.

Part of #24214, split from #31049 (last part! :) ).

The test for nth was added in #33103.

ranma42 added 4 commits May 28, 2016 20:29
@ranma42
Extract stepping from EscapeUnicode::next
1460667 
Extract a function that updates the iterator state and returns the
result of an arbitrary step of iteration.

This implements the same logic as `next`, but it can be shared with
`nth`.
@ranma42
Implement EscapeUnicode::nth
21bb7b8 
as a step from the appropriate state.

Part of rust-lang#24214.
@ranma42
Use the same structure for non-Unicode variants of EscapeDefaultState
0c71e95 
This makes it easier to have a unique path for handling all of them.
@ranma42
Unify EscapeDefault::next and EscapeDefault::nth
5d5b35c 
by extracting a shared `step` function.
@rust-highfive
Copy link
Contributor

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@ranma42
Copy link
Contributor Author

ranma42 commented May 28, 2016

Given that he has already reviewed the previous PRs, r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned brson May 28, 2016
@alexcrichton
Copy link
Member

This... seems like a lot of code for a pretty minor feature. Out of curiosity is there a crate which is actually exercising this in terms of desiring performance gains or are these just being done for completeness? If they're only done for completeness I'd somewhat prefer to hold off on the extra complexity for now.

@brson
Copy link
Contributor

brson commented May 31, 2016

Is it definitely a perf win?

I spent a while looking at this patch before deciding I didn't understand it enough to review. So I sort of agree with alex that it may not be worth the churn.

@ranma42
Copy link
Contributor Author

ranma42 commented Jun 1, 2016

A little less churn would be possible by implementing next as nth(0), but that would result in slightly worse code. Instead both are implemented on top of a step function, which the compiler inlines and constant-folds appropriately. In fact, most of the complexity of the code is related to moving the state switch from next to step so that it can be re-used.

The changes to EscapeDefaultState are merely code cleanup, as they unify the next and nth code paths, instead of keeping them duplicated. No performance change is expected there.

The implementation of nth for EscapeUnicode after this change becomes a simple switch (the same that would be used for next except for the constant offset) with no need to iterate over the successive values, so it would be easy to see major performance wins in artificial benchmarks. Nonetheless, I would be surprised if anybody used nth on EscapeUnicode values in actual code.

I agree that there is little use for this micro-optimisation (especially considered that the offsetting logic is not completely obvious at first sight), so it can probably be held off with no disadvantage.
I would love if it was possible to automatically get this kind of optimisation in generators, but I am afraid it will be very hard because of side-effects.

@alexcrichton
Copy link
Member

Ok, in that case it seems fine to close this for now and we can revisit if it becomes a perf hazard for someone?

@ranma42 ranma42 closed this Jun 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ranma42 @rust-highfive @alexcrichton @brson