Skip to content

testuite.yml - add PERL_SAWAMPERSAND testing #21304

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

Open
wants to merge 1 commit into
base: blead
Choose a base branch
from

Conversation

demerphq
Copy link
Collaborator

Make sure we test this build mode.

PERL_SAWAMPERSAND is related to how perl handles the use of $`, $& and
$' and the behavior of the regex engine. When it is defined then
certain slow copy operations are avoided UNLESS the code uses one of
these magic variables. This can have drammatic performance consequences
for certain code patterns, for example in while (//g) matching
against long strings.

When COW strings were introduced it was assumed that COW would be a
better solution to pessimizing the regex engine when these variables
were used, and for many code cases it is indeed more efficient. However
it is not a universal and consistent solution and there are cases where
matches that would be linear under PERL_SAWAMPERSAND are quadratic
without it. For those cases we support building perl with
PERL_SAWAMPERSAND defined so we do not rely on COW to deal with these
copy operations alone.

Since the default build is to not define PERL_SAWAMPERSAND code that
is governed by this build mode is not tested under normal
circumstanctes. This patch ensures that we test that code as well as
the default build mode.

From time to time people suggest that we remove PERL_SAWAMPERSAND and
simply rely on COW, however I believe that this is unwise. COW does not
apply to all strings, and COW only supports 254 copies of a string, so
some operations actually would end up quadratic. The situation is
unsatisfactory IMO, and until we can make COW more powerful than it is I
believe we should continue to support the escape hatch represented by
PERL_SAWAMPERSAND which implies testing it.
@bram-perl
Copy link

Can you elaborate (in the commit message) on why it's important to test this partciular configuration all the time?

@demerphq demerphq force-pushed the yves/test_sawampersand branch from 273787c to b53680b Compare July 27, 2023 19:35
@demerphq
Copy link
Collaborator Author

@bram-perl I think we should be testing all of our optional build modes all the time, so to me this is like asking "why do you bother with CI". But I have updated the commit regardles.

@khwilliamson
Copy link
Contributor

khwilliamson commented Jul 27, 2023 via email

@demerphq
Copy link
Collaborator Author

@khwilliamson The ideal is often impractical. My point was that I shouldn't have to explain why I am adding one to be tested. If we had infinite resources we would test them all. It is your call if you want to test your options or not. That you choose not to test some doesn't mean I should have to explain why I am testing another.

@bram-perl
Copy link

The point of explaining it (in the commit message) is so that people later know why it was done.

Let's assume that there is no reason in the commit message and that someone in the future decides to remove PERL_SAWAMPERSAND: then they'll notice that the testsuite fails and just remove the test configuration. After all there is no one that remembers why it was added in the first place.

@tonycoz
Copy link
Contributor

tonycoz commented Aug 1, 2023

I know there have been some efforts to reduce github workflow minute usage in the past, but I haven't been able to find any official limitations on public repositories.

See the testsuite.yml re-work from 430c0c0

@atoomic do you know if there are any limits?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants