Skip to content

Rewrite Numeric#times in Ruby using Primitive #9576

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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nobu
Copy link
Member

@nobu nobu commented Jan 17, 2024

No description provided.

@nobu nobu force-pushed the numeric-times-primitive branch from 4591663 to 0973f5d Compare January 17, 2024 09:25
@nobu nobu force-pushed the numeric-times-primitive branch from 0973f5d to d07f9f0 Compare January 17, 2024 09:41
@k0kubun
Copy link
Member

k0kubun commented Jan 18, 2024

Can you clarify the motivation? I don't think Matz asked for it https://bugs.ruby-lang.org/issues/20182#note-5:

If the user intentionally redefines the fundamental method, it should be his/her own responsibility.

Integer#< and Integer#succ are two things that are already optimized well on the interpreter and YJIT, and replacing them with a random cexpr! would only complicate YJIT so much to maintain the current performance. The redefinition concern seems much less meaningful than Array#each's race condition one, unless it's something like Integer#/.

I'm fine with replacing block_yield? with defined?(yield) since it's not Primitive though.

@nobu nobu marked this pull request as draft January 18, 2024 21:04
@nobu
Copy link
Member Author

nobu commented Jan 18, 2024

This was just an experiment, but I found that builtin-indexes were not working with while.
#9533 worked because it moved i.succ into ary_fetch_next by chance.
In that sense, this might be better to focus that bug and be earlier 2 commits only.

@k0kubun
Copy link
Member

k0kubun commented Jan 18, 2024

This was just an experiment, but I found that builtin-indexes were not working with while.
In that sense, this might be better to focus that bug and be earlier 2 commits only.

It'd be nice to have those builtin index fixes 👍

@nobu
Copy link
Member Author

nobu commented Jan 19, 2024

It'd be nice to have those builtin index fixes 👍

Are you OK with the restriction, cexpr! must be up to one per line?

@k0kubun
Copy link
Member

k0kubun commented Jan 19, 2024

Yeah, I'm fine with it as long as it gives an error on conflict. RJIT is an exception, but it'd usually mean that you should merge such primitives.

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.

2 participants