Skip to content

Remove mTHX, a badly conceived idea #23209

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

Merged
merged 16 commits into from
Apr 20, 2025
Merged

Remove mTHX, a badly conceived idea #23209

merged 16 commits into from
Apr 20, 2025

Conversation

khwilliamson
Copy link
Contributor

This fixes #22820

I came up with the idea of mTHX thinking it would make some functions able to become macros, pretty much emptying out mathoms.c. But the idea was flawed; it doesn't work for calls using the long Perl_foo form. This also resulted in #22902.

This series of commits simply reverts the ones that were used to convert to this construct, plus inlining 3 functions that came into existence after this construct, so there is nothing to revert. And it removes the relict documentation about it.

  • This set of changes does not require a perldelta entry.

@bulk88
Copy link
Contributor

bulk88 commented Apr 20, 2025

Without reading too deep on this commit, its fundamentally incompatible with Perl 5. mTHX is a my_perl var that needs to drop out on certain threaded perl build configs, and must be present on other threaded perl builds. "m" stands for the word memory or malloc. Somewhere on my todo list is removing dTHX; from inside Perl_safesysfree/Perl_safesysfree which are the P5P default build config ABI level front ends of Newx/Safefree and are super hot code paths for anyOS Perl and dTHX/Perl_get_context() is sadistic cruel joke on specifically WinPerl. I do have an many years old patch still open somewhere on RT/GH that did successfully remove dTHX; from inside Perl_safesysfree/Perl_safesysfree with core self test passing, but the ticket stalled out with yak-shaving/P5P is permanently closed/ " Lets Go Perl 6 "/risk of getting a just 1 BBC report from bizzare/poor C code.

Most common reasons why mTHX gets used are Newx/PerlIO/PERL_IMPLICIT_SYS/psuedo-fork/NOXSLOCKS/PERL_CORE vs wretched creatures from CPAN issues. On a CPP level, perl must drop out my_perl arg as an optimization for certain threaded perl builds where the my_perl arg will never be used in the body/impl of that perl C function for that particular build config permutation of that libperl.

@bulk88
Copy link
Contributor

bulk88 commented Apr 20, 2025

This fixes #22820

I came up with the idea of mTHX thinking it would make some functions able to become macros, pretty much emptying out mathoms.c. But the idea was flawed; it doesn't work for calls using the long Perl_foo form.

mathoms.c exists for totally nonsensical reasons, that were required for <= 5.10, back when P5P and stable perl guaranteed ABI binary compatibility with New Perl vs deployed Old CPAN XS .so/.dll files. The trolls in here will show up very shortly, to explain how mathoms.c prevents world wide economic collapse if CPAN XS libraries that NEVER #include "perl.h" in their TU but link against libperl.so and import C funcs from libperl, if these CPAN XS libs suddenly get broken, millions of orphans will starve to death in 3rd world countries.

@bulk88
Copy link
Contributor

bulk88 commented Apr 20, 2025

This fixes #22820
I came up with the idea of mTHX thinking it would make some functions able to become macros, pretty much emptying out mathoms.c. But the idea was flawed; it doesn't work for calls using the long Perl_foo form.

mathoms.c exists for totally nonsensical reasons, that were required for <= 5.10, back when P5P and stable perl guaranteed ABI binary compatibility with New Perl vs deployed Old CPAN XS .so/.dll files. The trolls in here will show up very shortly, to explain how mathoms.c prevents world wide economic collapse if CPAN XS libraries that NEVER #include "perl.h" in their TU but link against libperl.so and import C funcs from libperl, if these CPAN XS libs suddenly get broken, millions of orphans will starve to death in 3rd world countries.

PS, I have sometimes written code on cpan that uses the ancient no Perl_ or no perl prefixed larry era functions calls for perf reasons, to get rid of the last 1 or 0's arguments/cpu ops in my code, which are almost always 0s. I later changed my mind and turned on #NOMATHOMS on my dev box to save 350 milliseconds recompiling libperl.dll/miniperl.exe. It doesnt affect CPAN XS src code compatibility either way, since 1 is an extern "C" linkable symbol, and the NOMATHOMS are CPP macros or static inlines.

My irritation/someones irritation at Perl having alot of "useless" 0s in critical hot code paths, a better way of pleasuring this person would be targeting those extra useless 0s, C token by C token, and probably thinking up a custom localized context fix, or atleast documenting in src code, the no "_flags" variant will stay around under optimization rational, rather end users secretly using those identifiers in mathoms.c and not letting the community know about it.

These used the ill-advised mTHX construct which is being entirely
removed.
This construct was ill-advised; these comments are the final relics
mentioning it.
@jkeenan
Copy link
Contributor

jkeenan commented Apr 20, 2025

I have to admit that I have mixed feelings about this pull request, but mostly that stems from the fact that it's being filed on the day of our final regularly scheduled monthly development release in the 5.41 cycle. I would normally expect major changes to code deep in the Perl guts to have been resolved in the February or March releases.

However, the fact that the author of these code changes does not feel confident about going forward with them should signal to the rest of us that now is precisely the time to back them out and re-think the problem they were intended to solve during the next dev cycle. Either way, we can use comments from people familiar with this part of the code base and from PSC.

@tonycoz @ap @book @leonerd @Leont @haarg

@khwilliamson
Copy link
Contributor Author

I don't agree that reverting to an earlier state that worked for years constitutes a major change. That is why this had the lowest priority for me to get done. It doesn't have to go into this development release, but it does have to go into 5.42.

@khwilliamson
Copy link
Contributor Author

@bulk88 I think you are misunderstanding this. This PR is to restore things to a state that has persisted in Perl5 for a very long time so as to fix one known cpan bug, and to prevent darkpan ones

@haarg
Copy link
Contributor

haarg commented Apr 20, 2025

I think we should merge this before the 5.41.11 release.

@khwilliamson khwilliamson merged commit c308ac4 into Perl:blead Apr 20, 2025
33 checks passed
@khwilliamson khwilliamson deleted the mTHX branch April 23, 2025 10:33
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.

BBC: v5.41.6 breaks SADEGH/Crypt-Lucifer-0.1.tar.gz
4 participants