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

Optimise foreach on builtin::indexed ARRAY #22641

Open
wants to merge 5 commits into
base: blead
Choose a base branch
from

Conversation

leonerd
Copy link
Contributor

@leonerd leonerd commented Oct 2, 2024

Rather than generating an entire temporary list that is twice as big as the original array, instead set a flag on the OP_ITER that tells it to set one of the iteration variables to the current array index and use the same CXt_LOOP_ARY optimisastion that regular foreach over an array would use.

  • Consider whether builtin::indexed LIST... should do the same optimisation. Likely the answer is "yes", but in a separate change.

  • This set of changes requires a perldelta entry, and it is included

@leonerd
Copy link
Contributor Author

leonerd commented Oct 2, 2024

This change does have the potential impact that, before it, any modifications to the iterated-on array that happen during the body of its own foreach loop would not get seen, whereas now they would. This is the same issue that single-variable foreach on a plain array already had, so it's not new. But it is a change for existing code that already calls this function.

op.c Outdated Show resolved Hide resolved
lib/builtin.t Show resolved Hide resolved
@leonerd leonerd force-pushed the optimise-foreach-indexed branch 2 times, most recently from f3d8512 to 38d31c3 Compare October 9, 2024 19:30
…cals

Rather than generating an entire temporary list that is twice as big as
the original array, instead set a flag on the `OP_ITER` that tells it to
set one of the iteration variables to the current array index and use
the same `CXt_LOOP_ARY` optimisation that regular foreach over an array
would use.
@leonerd leonerd changed the title Optimise foreach on builtin::indexed Optimise foreach on builtin::indexed ARRAY Oct 28, 2024
@leonerd leonerd marked this pull request as ready for review October 28, 2024 09:57
@leonerd leonerd added the squash-before-merge Author must squash the commits down before merging to blead label Oct 28, 2024
Comment on lines +493 to +495
/* This does not use the XS() macro so that op.c can see its prototype */
void
Perl_XS_builtin_indexed(pTHX_ CV *cv)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

builtin.c: In function ‘Perl_XS_builtin_indexed’:
builtin.c:495:35: warning: unused parameter ‘cv’ [-Wunused-parameter]
  495 | Perl_XS_builtin_indexed(pTHX_ CV *cv)
      |                               ~~~~^~

Normally suppressed by the attribute in :

#define XSPROTO(name) void name(pTHX_ CV* cv __attribute__unused__)

@tonycoz
Copy link
Contributor

tonycoz commented Oct 29, 2024

Deparse needs an update:

$ ./perl -Ilib -MO=Deparse -Mbuiltin=indexed -e 'for my ($i, $v) (indexed @ARGV) { print "$i: $v\n"; }' 1 2 3
use builtin (split(/,/, 'indexed', 0));
foreach my ($i, $v) (@ARGV) {
    print "${i}: $v\n";
}
-e syntax OK

@tonycoz
Copy link
Contributor

tonycoz commented Oct 29, 2024

Looks ok otherwise

@Grinnz
Copy link
Contributor

Grinnz commented Oct 29, 2024

This change does have the potential impact that, before it, any modifications to the iterated-on array that happen during the body of its own foreach loop would not get seen, whereas now they would. This is the same issue that single-variable foreach on a plain array already had, so it's not new. But it is a change for existing code that already calls this function.

Consistency seems good, and the foreach docs basically say "don't do this" already. But maybe this change should have a brief mention in perldelta, since this feature isnt experimental anymore?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squash-before-merge Author must squash the commits down before merging to blead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants