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.

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

replace zip_eq with optimizable version #7779

Closed
kwannoel opened this issue Feb 8, 2023 · 8 comments · Fixed by #7795
Closed

replace zip_eq with optimizable version #7779

kwannoel opened this issue Feb 8, 2023 · 8 comments · Fixed by #7795
Assignees
@xxchan
Labels
type/feature
Milestone
release-0.1.17

Comments

@kwannoel
Copy link
Contributor

kwannoel commented Feb 8, 2023 •
edited
Loading

As mentioned by @BugenZhao : #7749 (comment)

And previously caught by @wangrunji0408 in #6856.

We should probably replace zip_eq system wide with something else. This should be optimized away in release and bench builds.
@github-actions github-actions bot added this to the release-0.1.17 milestone Feb 8, 2023
@kwannoel kwannoel self-assigned this Feb 8, 2023
@kwannoel
Copy link
Contributor Author

kwannoel commented Feb 8, 2023

cc @jon-chuang

@BugenZhao
Copy link
Member

In my opinion, I suggest still making it an extension method to Iterator, named something like zip_debug_eq, and returning an opaque type of impl Iterator<..>, so we can return Zip when debug assertions are off and ZipEq when on. We may furthermore make a pull request to the upstream about it. 🥵

@xxchan
Copy link
Member

xxchan commented Feb 8, 2023

I failed 🥵😭

image

@BugenZhao
Copy link
Member

I failed 🥵😭

We can't do that without specialization. 😭

@xxchan
Copy link
Member

xxchan commented Feb 8, 2023

Let me do a stupid version instead

@kwannoel
Copy link
Contributor Author

kwannoel commented Feb 8, 2023

Let me do a stupid version instead

I'm planning to implement it in #7749 👀

@xxchan
Copy link
Member

xxchan commented Feb 8, 2023 •
edited
Loading

Did you begin? I can do the dirty work if not 🥵

@kwannoel
Copy link
Contributor Author

kwannoel commented Feb 8, 2023 •
edited
Loading

Did you begin? I can finish the dirty work 🥵

Okay you can go ahead :) Thanks!

@kwannoel kwannoel assigned xxchan and unassigned kwannoel Feb 8, 2023
@xxchan xxchan mentioned this issue Feb 8, 2023
Merged
2 tasks
@mergify mergify bot closed this as completed in #7795 Feb 8, 2023
mergify bot pushed a commit that referenced this issue Feb 8, 2023
@xxchan
perf: replace zip_eq with zip_eq_fast/zip_eq_debug (#7795)
312da1f 
close #7779

See the rustdoc in `src/common/src/util/iter_util.rs` for motivation.

Approved-By: BugenZhao
Approved-By: kwannoel
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@xxchan xxchan

Labels
type/feature
Projects
None yet
Milestone
release-0.1.17
Development

Successfully merging a pull request may close this issue.

perf: replace zip_eq with zip_eq_fast/zip_eq_debug risingwavelabs/risingwave
3 participants
@BugenZhao @xxchan @kwannoel