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

Fix some indirect calls in threads #120

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

Fix some indirect calls in threads #120

wants to merge 1 commit into from

Conversation

atoomic
Copy link
Owner

@atoomic atoomic commented Jul 16, 2020

Fixes #12

@atoomic atoomic requested a review from jkeenan July 16, 2020 21:59
@jkeenan jkeenan changed the title Fix some indirect calls in threds Fix some indirect calls in threads Jul 16, 2020
@jkeenan
Copy link
Collaborator

jkeenan commented Jul 16, 2020

Okay, but ... those were just 2 files I cited as examples. The pattern was occurring 276 times in the test suite.

So I suggest we just hold this p.r. here. I'd like to go back and run a complete make test_harness and assess the status of #12. Then we'll be able to see how the fixes you suggest for these two files can be applied on scale.

Thank you very much.
Jim Keenan

@atoomic
Copy link
Owner Author

atoomic commented Jul 20, 2020

@jkeenan I understand that there are more files than only these two, but nothing block us from fixing these two?
This is just a partial fix for #12 and not a complete fix. IMO we could use different PRs to fix this.

@jkeenan
Copy link
Collaborator

jkeenan commented Jul 22, 2020

@atoomic, what I've done is to apply your patch to a new branch which I am testing locally. Getting indirect object syntax out of threads.pm is an obvious point of first entry for working on threads more generally. I will run a complete make test on this and report results.

Thank you very much.
Jim Keenan

@jkeenan
Copy link
Collaborator

jkeenan commented Jul 22, 2020

@atoomic, what I've done is to apply your patch to a new branch which I am testing locally. Getting indirect object syntax out of threads.pm is an obvious point of first entry for working on threads more generally. I will run a complete make test on this and report results.

Thank you very much.
Jim Keenan

I created a local branch by applying your patch, then did a simple threaded build: sh ./Configure -des -Dusedevel -Duseithreads. I then ran make test_prep, captured the output and examined it for new build-time warnings. There were a few, but I'll open a separate ticket for those.

I then ran make test and captured the output. As was the case with a previous attempt to test a threaded build, the run took 49 minutes (compared to 15 for unthreaded), probably due to timeouts. In addition, there were vastly many more test failures. Failed 419 tests out of 2322, 81.96% okay. This compares to about 90% passing in p7.

  1. Many test files are skipped on unthreaded builds, so this is the first time we have addressed them.

  2. Many tests that were passing and running clean on threaded are now failing. And these are in "basic" directories like t/re and t/comp.

So my feeling is this: Yes, we can merge this branch into p7. But since that will wipe out much of our (nominal) progress in the past three weeks, if we do that, then we have to make getting threaded builds running clean our top priority. Which means we both have to work on this and probably recruit additional eyes as well.

And, of course, making threads top priority means other things get bumped to lower priority. But since we can, to a certain extent, kick the can down the road on, say, subroutine-prototype-confusion, we cannot do that with threads. We would have to have an "all hands on deck" attitude toward this challenge.

If you're okay with that, then proceed to merge this branch into p7 and we'll deal with the additional test failures.

Thank you very much.
Jim Keenan

@atoomic
Copy link
Owner Author

atoomic commented Jul 22, 2020

Will definitively have to check what s wrong with threads

@jkeenan
Copy link
Collaborator

jkeenan commented Sep 28, 2020

@atoomic, I think we resolved the problems with threads a couple of months ago. And "no-feature-indirect-by-default" is no longer on our roadmap. So I think that makes this p.r. closable. Do you agree?

Thank you very much.
Jim Keenan

@atoomic
Copy link
Owner Author

atoomic commented Sep 28, 2020

Even if indirect is not going to become the default, core code and thus dual life should avoid using indirect calls IMO
this change https://github.com/atoomic/perl/pull/120/files is definitively good to have and then have to be submitted upstream
if this is not the case already

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.

Error: "Bareword found where operator expected ... Do you need to predeclare import?"
2 participants