Closed p5pRT closed 13 years ago
We find it convenient to construct dynamic packages in our code for various purposes\, and have created internal tools for manipulating these packages. We have found that the use of these packages seems to be associated with memory leaks which impair our code's operation when it runs for weeks or months at a time\, and believe we have tracked the problem to a deficiency in Symbol::delete_package.
When we call Symbol::delete_package\, we expect that all traces of the package in question would disappear. (We recognize that the package is not removed from %INC and manage to do so ourselves.) However\, it seems that there is still some memory left over which is not freed. The memory appears to be located somewhere in Perl-guts (we have tried to track it with Devel::Leak and found nothing) and seems to be associated with the contents of the magic @ISA variable in the package: somewhere about 80 bytes per package if @ISA contains a single empty string\, more if there is a more complicated inheritance pattern\, none if @ISA is the empty list.
A script to reproduce this issue follows. This code not only leaks memory on our internal build of Perl 5.10.1\, but we have also found it to leak on stock perl 5.8.8 from CentOS and on stock perl 5.10.1 on Ubuntu 10.04. (The script uses the Linux/proc/ filesystem as a convenient way to display memory usage; I'm sure that it can be adapted to another platform readily enough).
use strict; use warnings;
use Symbol; # use Devel::Leak;
@a::ISA = qw(UNIVERSAL); @b::ISA = qw(a); @c::ISA = qw(b);
my $leak_more = 1; my $base = $leak_more ? 'c' : '';
# my $handle; # Devel::Leak::NoteSV($handle);
for my $count (0..20_000) { no strict 'refs'; *{"Foo::${count}::ISA"} = [$base]; Symbol::delete_package("Foo::${count}");
memstats() unless $count % 5000; }
# Devel::Leak::CheckSV($handle);
sub memstats { system("cat/proc/$$/status | grep VmRSS"); }
For completeness (and to see whether an upgrade would make the problem just "go away") we compiled a version of Perl 5.12.0 and ran the script there as well\, but the memory leak was still present.
dev@airwave.com - Status changed from 'new' to 'open'
whoops! i think that's *your* workflow I just clicked.
dev@airwave.com - Status changed from 'open' to 'new'
This was caused by change 31239/70cd14 which fixes *ISA=[] to call mro_isa_changed_in.
The actual underlying cause is explained in mroâs manpage\, under mro::get_isarev:
Currently\, this list only grows\, it never shrinks. This was a performance consideration (properly tracking and deleting isarev entries when someone removes an entry from an @ISA is costly\, and it doesn't happen often anyways).
If it doesnât happen often\, then the cost will not be incurred very often.
Normally\, the list of parent classes is cached\, so Perl_mro_isa_changed_in can be made to compare it with the new list and delete isarev entries as appropriate. This may not be very efficient\, but\, again\, it wonât happen very often\, as the existing cached list will normally be empty.
S_hfreeentries is the only other routine that clears the cache\, so I can make that remove isarev entries\, too.
If I write a patch for this\, is there any remote possibility of its being applied\, or would it be a waste of time?
Another possibility for fixing this particular case is to add a function to mro:: for deleting isarev entries for particular classes. Then Symbol::delete_package could be made to use that.
The RT System itself - Status changed from 'new' to 'open'
On Sun\, Jul 25\, 2010 at 12:57:02PM -0700\, webmasters@ctosonline.org wrote:
This was caused by change 31239/70cd14 which fixes *ISA=[] to call mro_isa_changed_in.
The actual underlying cause is explained in mro?s manpage\, under mro::get_isarev:
Currently\, this list only grows\, it never shrinks\. This was a performance consideration \(properly tracking and deleting isarev entries when someone removes an entry from an @​ISA is costly\, and it doesn't happen often anyways\)\.
If it doesn?t happen often\, then the cost will not be incurred very often.
Normally\, the list of parent classes is cached\, so Perl_mro_isa_changed_in can be made to compare it with the new list and delete isarev entries as appropriate. This may not be very efficient\, but\, again\, it won?t happen very often\, as the existing cached list will normally be empty.
S_hfreeentries is the only other routine that clears the cache\, so I can make that remove isarev entries\, too.
If I write a patch for this\, is there any remote possibility of its being applied\, or would it be a waste of time?
The documentation you quoted continues with:
The fact that a class which no longer truly "isa" this class at runtime remains on the list should be considered a quirky implementation detail which is subject to future change. It shouldn't be an issue as long as you're looking at this list for the same reasons the core code does: as a performance optimization over having to search every class in existence.
Yes\, finding an efficient way to fix this would be useful and welcomed.
Nicholas Clark
On Jul 27\, 2010\, at 7:25 AM\, Nicholas Clark wrote:
On Sun\, Jul 25\, 2010 at 12:57:02PM -0700\, webmasters@ctosonline.org wrote:
This was caused by change 31239/70cd14 which fixes *ISA=[] to call mro_isa_changed_in.
The actual underlying cause is explained in mro?s manpage\, under mro::get_isarev:
Currently\, this list only grows\, it never shrinks\. This was a performance consideration \(properly tracking and deleting isarev entries when someone removes an entry from an @​ISA is costly\, and it doesn't happen often anyways\)\.
If it doesn?t happen often\, then the cost will not be incurred very often.
Normally\, the list of parent classes is cached\, so Perl_mro_isa_changed_in can be made to compare it with the new list and delete isarev entries as appropriate. This may not be very efficient\, but\, again\, it won?t happen very often\, as the existing cached list will normally be empty.
S_hfreeentries is the only other routine that clears the cache\, so I can make that remove isarev entries\, too.
If I write a patch for this\, is there any remote possibility of its being applied\, or would it be a waste of time?
The documentation you quoted continues with:
The fact that a class which no longer truly "isa" this class at runtime remains on the list should be considered a quirky implementation detail which is subject to future change. It shouldn't be an issue as long as you're looking at this list for the same reasons the core code does: as a performance optimization over having to search every class in existence.
Yes\, finding an efficient way to fix this would be useful and welcomed.
OK\, now for the next set of questions:
This script shows a regression in 5.10\, with regard to stash assignments. I can rearrange packages to my heartâs content without triggering mro_isa_changed_in.
@a'ISA = 'b'; @b'ISA = 'c';
sub c'bark { warn "Woof!" } sub d'bark { warn "Bow-wow!" }
$a = bless []\, a; $a->bark; # Woof!
@e'ISA = 'd'; $b = delete $::{'b::'}; $::{'b::'} = delete $::{'e::'};
$a->bark; # Woof! in 5.10; Bow-wow! in 5.8 undef $b; $a->bark; # Bow-wow!
print *{"b::"}\,"\n"; # Shows that it still thinks it is *e::
For my proposed isarev changes to work\, every stash (or the glob containing the stash) needs to know whether it is still part of the main stash hierarchy\, or whether it has been orphaned\, so that it wonât delete isarev entries that do not belong to it.
In the script above\, âdelete $::{'b::'}â needs to trigger mro_isa_changed_in and the return value flagged such that it knows not to remove isarev entries when its @ISA is changed.
For instance\, c and d could both inherit from f. Without such a flag\, @{*{$$b{'ISA::'}}{ARRAY}} = () will remove a from fâs isarev\, which shouldnât happen.
The flags on SVs seem to be a rather crowded space. Am I right in thinking hashes cannot have get-magic? If that is the case\, I can re-use SVs_GMG for this purpose.
And then each stash needs to know its effective name\, as opposed to its original name (see the last line of the script above). What would be the best way to do this? Store it in magic?
On Sun\, Aug 01\, 2010 at 12:17:36PM -0700\, Father Chrysostomos wrote:
[snip pathological but interesting example]
The flags on SVs seem to be a rather crowded space. Am I right in thinking hashes cannot have get-magic? If that is the case\, I can re-use SVs_GMG for this purpose.
I don't know with 100% certainty\, but I see tests on SvGMAGICAL() in hv.c\, this in mg.c:
if (vtbl->svt_get && !(mg->mg_flags & MGf_GSKIP)) SvGMAGICAL_on(sv);
and magic vtables in perl.h which have the svt_get function pointer set\, such as
MGVTBL_SET( PL_vtbl_sigelem\, MEMBER_TO_FPTR(Perl_magic_getsig)\, MEMBER_TO_FPTR(Perl_magic_setsig)\, 0\, MEMBER_TO_FPTR(Perl_magic_clearsig)\, 0\, 0\, 0\, 0 );
so I'm assuming that yes\, it is used. To the best of my knowledge there are no free flag bits on PVHV\, and no more that can be scavenged.
And then each stash needs to know its effective name\, as opposed to its original name (see the last line of the script above). What would be the best way to do this? Store it in magic?
Having any magic on all stashes is a measurable performance hit. For a while in 5.9.x there was magic on stashes to store weak reference backreferences (no vtable actions for get or set) and that was enough to slow things down. That's why there's all the "extra" code to allow hashes to store backreferences in the hv_aux structure. I believe that that's the logical place to hang any extra data needed for this.
Also\, if it's easier to implement\, I don't see a problem with using the global sub generation counter to flag when someone has directly manipulated the tree of stashes. It's not common\, and I don't see an absolute need to calculate the strict set of packages affected\, and call mro_changed_in() for just them.
Nicholas Clark
On Aug 2\, 2010\, at 2:34 AM\, Nicholas Clark wrote:
That's why there's all the "extra" code to allow hashes to store backreferences in the hv_aux structure. I believe that that's the logical place to hang any extra data needed for this.
If you are suggesting adding new fields to the xpvhv_aux struct\, what do I need to know about alignment issues?
(After doing some research\, it now looks as though I just need one more field; viz.\, a HEK* to store an alternate name.)
Also\, if it's easier to implement\, I don't see a problem with using the global sub generation counter to flag when someone has directly manipulated the tree of stashes.
Doesnât that just affect method caches? What about isa caches?
Looking through the isa code\, I havenât yet seen anything that checks PL_sub_generation\, but I may have missed it.
If PL_sub_generation does not invalidate isa caches\, would it be appropriate to add a new PL_isa_generation variable?
It's not common\, and I don't see an absolute need to calculate the strict set of packages affected\, and call mro_changed_in() for just them.
And I now realise that such would be very inefficient anyway\, as nested classes have nothing to do with inheritance. (*p:: = *PPI:: would have to iterate through 94 packages\, recalculating the linear isa cache\, etc. *p:: = delete $::{"PPI"} would do that iteration twice.)
Father Chrysostomos
On Aug 8\, 2010\, at 12:22 PM\, Father Chrysostomos wrote:
And I now realise that such would be very inefficient anyway\, as nested classes have nothing to do with inheritance. (*p:: = *PPI:: would have to iterate through 94 packages\, recalculating the linear isa cache\, etc. *p:: = delete $::{"PPI"} would do that iteration twice.)
Oops. I meant delete $::{"PPI::"}\, of course.
On Sun\, Aug 08\, 2010 at 12:22:13PM -0700\, Father Chrysostomos wrote:
If you are suggesting adding new fields to the xpvhv_aux struct\, what do I need to know about alignment issues?
(After doing some research\, it now looks as though I just need one more field; viz.\, a HEK* to store an alternate name.)
Note that hv_clear and hv_undef may remove the aux struct\, so you may not be able to rely on it always remaining.
-- In England there is a special word which means the last sunshine of the summer. That word is "spring".
On Aug 18\, 2010\, at 9:47 AM\, Dave Mitchell wrote:
On Sun\, Aug 08\, 2010 at 12:22:13PM -0700\, Father Chrysostomos wrote:
If you are suggesting adding new fields to the xpvhv_aux struct\, what do I need to know about alignment issues?
(After doing some research\, it now looks as though I just need one more field; viz.\, a HEK* to store an alternate name.)
Note that hv_clear and hv_undef may remove the aux struct\, so you may not be able to rely on it always remaining.
What happens to the name currently stored in there when that happens?
On Aug 8\, 2010\, at 12:22 PM\, Father Chrysostomos wrote:
If PL_sub_generation does not invalidate isa caches\, would it be appropriate to add a new PL_isa_generation variable?
It's not common\, and I don't see an absolute need to calculate the strict set of packages affected\, and call mro_changed_in() for just them.
And I now realise that such would be very inefficient anyway\, as nested classes have nothing to do with inheritance. (*p:: = *PPI:: would have to iterate through 94 packages\, recalculating the linear isa cache\, etc. *p:: = delete $::{"PPI"} would do that iteration twice.)
I think we will have to live with this inefficiency. Anyway\, most of the time I alias packages\, itâs just for a few. The most common example in my code is âuse DDSâ which loads DDS.pm\, which aliases DDS:: to Data'Dump'Streamer::.
Oops. I forgot the main part of the message and just sent the postscript. Here it is:
Begin forwarded message:
From: Father Chrysostomos Date: August 22\, 2010 5:39:15 PM PDT Cc: Nicholas Clark\, p5p Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
On Aug 8\, 2010\, at 12:22 PM\, Father Chrysostomos wrote:
If PL_sub_generation does not invalidate isa caches\, would it be appropriate to add a new PL_isa_generation variable?
I tried PL_sub_generation\, and found that it doesnât work. I tried adding PL_isa_generation (attached\, in case you want to see it; not for application)\, which worked\, but then I realised that it would mean iterating through *all* packages afterwards to reconstruct PL_isarev.
So Iâve used a different approach with the other patch attached (1. reset isa...). When a stash is manipulated\, it calls mro_isa_changed_in on the affected packages and iterates through their subpackages\, doing the same.
It does not yet take into account all methods of manipulating the stashes. More patches will follow.
What does this have to do with the original bug?
Initially I planned to fix the isarev leaks by making appropriate adjustments to it whenever an @ISA is modified or a stash is freed. But\, since stashes can be detached from the main tree of stashes ($stash = delete $::{"it::"})\, they need to know whether they are still attached\, so detached stashes donât muddle up isarev and delete entries that are still needed (which would be worse than leaking).
In the process of experimenting with that\, I found that such stash manipulations already fail to update isa caches\, which\, too\, would cause my proposed isarev changes to introduce regressions. So thatâs what this patch is for.
It's not common\, and I don't see an absolute need to calculate the strict set of packages affected\, and call mro_changed_in() for just them.
And I now realise that such would be very inefficient anyway\, as nested classes have nothing to do with inheritance. (*p:: = *PPI:: would have to iterate through 94 packages\, recalculating the linear isa cache\, etc. *p:: = delete $::{"PPI"} would do that iteration twice.)
I think we will have to live with this inefficiency. Anyway\, most of the time I alias packages\, itâs just for a few. The most common example in my code is âuse DDSâ which loads DDS.pm\, which aliases DDS:: to Data'Dump'Streamer::.
On Aug 22\, 2010\, at 5:46 PM\, Father Chrysostomos wrote:
So Iâve used a different approach with the other patch attached (1. reset isa...). When a stash is manipulated\, it calls mro_isa_changed_in on the affected packages and iterates through their subpackages\, doing the same.
And I forgot to add the new test to MANIFEST. Hereâs a patch for that.
On Sun\, Aug 22\, 2010 at 12:11:27PM -0700\, Father Chrysostomos wrote:
On Aug 18\, 2010\, at 9:47 AM\, Dave Mitchell wrote:
On Sun\, Aug 08\, 2010 at 12:22:13PM -0700\, Father Chrysostomos wrote:
If you are suggesting adding new fields to the xpvhv_aux struct\, what do I need to know about alignment issues?
(After doing some research\, it now looks as though I just need one more field; viz.\, a HEK* to store an alternate name.)
Note that hv_clear and hv_undef may remove the aux struct\, so you may not be able to rely on it always remaining.
What happens to the name currently stored in there when that happens?
See S_hfreeentries: it special-cases adding name back at the end.
-- Wesley Crusher gets beaten up by his classmates for being a smarmy git\, and consequently has a go at making some friends of his own age for a change. -- Things That Never Happen in "Star Trek" #18
On Mon\, Aug 23\, 2010 at 12:50:20PM +0100\, Dave Mitchell wrote:
See S_hfreeentries: it special-cases adding name back at the end.
In fact I'm tempted to modify this code so that the aux structure is always maintained (ecvne for hv_undef) when there's something that nedds to be kept in it\, so that back-references no longer have to be shuffled betwween hv_aux and magic.
-- In economics\, the exam questions are the same every year. They just change the answers.
On Aug 22\, 2010\, at 5:46 PM\, Father Chrysostomos wrote:
Initially I planned to fix the isarev leaks by making appropriate adjustments to it whenever an @ISA is modified or a stash is freed. But\, since stashes can be detached from the main tree of stashes ($stash = delete $::{"it::"})\, they need to know whether they are still attached\, so detached stashes donât muddle up isarev and delete entries that are still needed (which would be worse than leaking).
In the process of experimenting with that\, I found that such stash manipulations already fail to update isa caches\, which\, too\, would cause my proposed isarev changes to introduce regressions. So thatâs what this patch is for.
Here is a patch that deals with a couple more ways of manipulating stashes.
I plan to work on assignment to empty stash elements next (or any stash elements that are not globs). I can do this by using PVLVs. But there are two approaches:
1. Do it just for access from Perl by adding it to pp_helem. 2. Do it for XS access as well\, by modifying hv_fetch_ent instead (or hv_common\, or wherever the actual code is).
Number 1 makes it too easy for XS code to make changes without triggering mro_package_moved. mro_package_moved will also have to be made public (and probably given a better name). Number 2 is probably more reliable. perlâs internals can pass a flag to avoid the PVLVs. XS code that assumes that stash elements are never magical will be broken (code that specifically calls the _nomg forms to avoid the magic checks). Most XS code will continue to work.
So in both cases\, XS code will have to be modified (if there is any that plays with stashes). But in case 2 only code that is currently potentially buggy will have to change.
I prefer number 2.
From: Father Chrysostomos \sprout@​cpan\.org
[perl #75176] Make more ways to manipulate stashes reset isa caches
This makes string-to-glob assignment and hashref-to-glob assignment reset isa caches by calling mro_package_moved\, if the globâs name ends with ::.
On 29 August 2010 23:09\, Father Chrysostomos \sprout@​cpan\.org wrote:
On Aug 22\, 2010\, at 5:46 PM\, Father Chrysostomos wrote:
Initially I planned to fix the isarev leaks by making appropriate adjustments to it whenever an @ISA is modified or a stash is freed. But\, since stashes can be detached from the main tree of stashes ($stash = delete $::{"it::"})\, they need to know whether they are still attached\, so detached stashes donât muddle up isarev and delete entries that are still needed (which would be worse than leaking).
In the process of experimenting with that\, I found that such stash manipulations already fail to update isa caches\, which\, too\, would cause my proposed isarev changes to introduce regressions. So thatâs what this patch is for.
Here is a patch that deals with a couple more ways of manipulating stashes. I plan to work on assignment to empty stash elements next (or any stash elements that are not globs). I can do this by using PVLVs. But there are two approaches: 1. Do it just for access from Perl by adding it to pp_helem. 2. Do it for XS access as well\, by modifying hv_fetch_ent instead (or hv_common\, or wherever the actual code is). Number 1 makes it too easy for XS code to make changes without triggering mro_package_moved. mro_package_moved will also have to be made public (and probably given a better name). Number 2 is probably more reliable. perlâs internals can pass a flag to avoid the PVLVs. XS code that assumes that stash elements are never magical will be broken (code that specifically calls the _nomg forms to avoid the magic checks). Most XS code will continue to work. So in both cases\, XS code will have to be modified (if there is any that plays with stashes). But in case 2 only code that is currently potentially buggy will have to change. I prefer number 2.
+ if (stype == SVt_PVHV) { + const char * const name = GvNAME((GV*)dstr); + const STRLEN len = GvNAMELEN(dstr); + if (len > 1 && name[len-2] == ':' && name[len-1] == ':') + if(HvNAME(dref)) mro_package_moved((HV *)dref); + if(HvNAME(sref)) mro_package_moved((HV *)sref); + }
Did you miss a { } there? From the indenting it looks like the if () applies to both of the following ifs\, but it is an if (EXPR) STMT; not an IF (EXPR) BLOCK
yves
-- perl -Mre=debug -e "/just|another|perl|hacker/"
On Aug 29\, 2010\, at 3:20 PM\, demerphq wrote:
On 29 August 2010 23:09\, Father Chrysostomos \sprout@​cpan\.org wrote: + if (stype == SVt_PVHV) { + const char * const name = GvNAME((GV*)dstr); + const STRLEN len = GvNAMELEN(dstr); + if (len > 1 && name[len-2] == ':' && name[len-1] == ':') + if(HvNAME(dref)) mro_package_moved((HV *)dref); + if(HvNAME(sref)) mro_package_moved((HV *)sref); + }
Did you miss a { } there? From the indenting it looks like the if () applies to both of the following ifs\, but it is an if (EXPR) STMT; not an IF (EXPR) BLOCK
Thank you for catching that. Here is a fixed version.
From: Father Chrysostomos \sprout@​cpan\.org
[perl #75176] Make more ways to manipulate stashes reset isa caches
This makes string-to-glob assignment and hashref-to-glob assignment reset isa caches by calling mro_package_moved\, if the globâs name ends with ::.
On Aug 22\, 2010\, at 5:46 PM\, Father Chrysostomos wrote:
Oops. I forgot the main part of the message and just sent the postscript. Here it is:
Begin forwarded message:
From: Father Chrysostomos Date: August 22\, 2010 5:39:15 PM PDT Cc: Nicholas Clark\, p5p Subject: Re: [perl #75176] Symbol::delete_package does not free certain memory associated with package::ISA
On Aug 8\, 2010\, at 12:22 PM\, Father Chrysostomos wrote:
If PL_sub_generation does not invalidate isa caches\, would it be appropriate to add a new PL_isa_generation variable?
I tried PL_sub_generation\, and found that it doesnât work. I tried adding PL_isa_generation (attached\, in case you want to see it; not for application)\, which worked\, but then I realised that it would mean iterating through *all* packages afterwards to reconstruct PL_isarev.
So Iâve used a different approach with the other patch attached (1. reset isa...). When a stash is manipulated\, it calls mro_isa_changed_in on the affected packages and iterates through their subpackages\, doing the same.
That patch can cause a bus error\, so here is a new version. The third patch no longer applies with this version\, so here is a new version of that\, too (3b).
From: Father Chrysostomos \sprout@​cpan\.org
[perl #75176] Reset isa on stash manipulation
This only applies to glob-to-glob assignments and deletions of stash elements. Other types of stash manipulation are dealt with by subse- quent patches.
It adds mro_package_moved\, a private function that iterates through subpackages\, calling mro_isa_changed_in on each.
From: Father Chrysostomos \sprout@​cpan\.org
[perl #75176] Make more ways to manipulate stashes reset isa caches
This makes string-to-glob assignment and hashref-to-glob assignment reset isa caches by calling mro_package_moved\, if the globâs name ends with ::.
On Aug 29\, 2010\, at 2:09 PM\, Father Chrysostomos wrote:
On Aug 22\, 2010\, at 5:46 PM\, Father Chrysostomos wrote:
Initially I planned to fix the isarev leaks by making appropriate adjustments to it whenever an @ISA is modified or a stash is freed. But\, since stashes can be detached from the main tree of stashes ($stash = delete $::{"it::"})\, they need to know whether they are still attached\, so detached stashes donât muddle up isarev and delete entries that are still needed (which would be worse than leaking).
In the process of experimenting with that\, I found that such stash manipulations already fail to update isa caches\, which\, too\, would cause my proposed isarev changes to introduce regressions. So thatâs what this patch is for.
Here is a patch that deals with a couple more ways of manipulating stashes.
I plan to work on assignment to empty stash elements next (or any stash elements that are not globs). I can do this by using PVLVs. But there are two approaches:
1. Do it just for access from Perl by adding it to pp_helem. 2. Do it for XS access as well\, by modifying hv_fetch_ent instead (or hv_common\, or wherever the actual code is).
Number 1 makes it too easy for XS code to make changes without triggering mro_package_moved. mro_package_moved will also have to be made public (and probably given a better name). Number 2 is probably more reliable. perlâs internals can pass a flag to avoid the PVLVs. XS code that assumes that stash elements are never magical will be broken (code that specifically calls the _nomg forms to avoid the magic checks). Most XS code will continue to work.
So in both cases\, XS code will have to be modified (if there is any that plays with stashes). But in case 2 only code that is currently potentially buggy will have to change.
I prefer number 2.
Here is a patch for this. I used approach number 1 because number 2 proved to be far more complicated than I had thought\, and probably too risky. I am not so sure now that mro_package_moved needs to be made public. But we could change that later if necessary.
I did not bother with a mathom for mro_isa_changed_in\, since this patch relies on the fix for #77362\, which changes behaviour in a way that is not suitable for maint. (There are many other bugs that can occur more often as a result of that fix.)
From: Father Chrysostomos \sprout@​cpan\.org
[perl #75176] Make assignment to undef stash elems reset caches
This makes assignment to undefined or nonexistent stash elements invalidate isa and method caches on the packages affected.
It does this by creating PVLVs with defelem magic in pp_helem if the current value of the stash element is not a real (!SvFAKE) glob. This\, of course\, only applies to keys ending with ::.
Because it is necessary to invalidate isa caches on the subclasses of a nonexistent package when its corresponding stash element is assigned to\, mro_isa_changed_in is replaced with mro_isa_changed_in3. See the docs for it in the diff for mro.c and its use in mg.c.
On Sun Sep 05 13:15:28 2010\, sprout wrote:
On Aug 22\, 2010\, at 5:46 PM\, Father Chrysostomos wrote:
So Iâve used a different approach with the other patch attached (1. reset isa...). When a stash is manipulated\, it calls mro_isa_changed_in on the affected packages and iterates through their subpackages\, doing the same.
That patch can cause a bus error\, so here is a new version.
Applied as c8bbf675c.
On Sun Sep 05 13:15:28 2010\, sprout wrote:
On Aug 22\, 2010\, at 5:46 PM\, Father Chrysostomos wrote:
So Iâve used a different approach with the other patch attached (1. reset isa...). When a stash is manipulated\, it calls mro_isa_changed_in on the affected packages and iterates through their subpackages\, doing the same.
That patch can cause a bus error\, so here is a new version.
Applied as c8bbf675c.
On Sun Aug 22 20:21:25 2010\, sprout wrote:
On Aug 22\, 2010\, at 5:46 PM\, Father Chrysostomos wrote:
So Iâve used a different approach with the other patch attached (1. reset isa...). When a stash is manipulated\, it calls mro_isa_changed_in on the affected packages and iterates through their subpackages\, doing the same.
And I forgot to add the new test to MANIFEST. Hereâs a patch for that.
Iâm now putting the tests in package_alias.t or whatever itâs called\, so that patch is no longer applicable.
On Sun Aug 22 20:21:25 2010\, sprout wrote:
On Aug 22\, 2010\, at 5:46 PM\, Father Chrysostomos wrote:
So Iâve used a different approach with the other patch attached (1. reset isa...). When a stash is manipulated\, it calls mro_isa_changed_in on the affected packages and iterates through their subpackages\, doing the same.
And I forgot to add the new test to MANIFEST. Hereâs a patch for that.
Iâm now putting the tests in package_alias.t or whatever itâs called\, so that patch is no longer applicable.
On Sun Sep 05 13:15:28 2010\, sprout wrote:
That patch can cause a bus error\, so here is a new version. The third patch no longer applies with this version\, so here is a new version of that\, too (3b).
3b has been applied as 1c93aaac75354eeef.
On Sun Sep 05 13:15:28 2010\, sprout wrote:
That patch can cause a bus error\, so here is a new version. The third patch no longer applies with this version\, so here is a new version of that\, too (3b).
3b has been applied as 1c93aaac75354eeef.
On Sat Oct 09 23:06:49 2010\, sprout wrote:
On Sun Sep 05 13:15:28 2010\, sprout wrote:
That patch can cause a bus error\, so here is a new version. The third patch no longer applies with this version\, so here is a new version of that\, too (3b).
3b has been applied as 1c93aaac75354eeef.
Er\, actually 3e79609f389.
On Sat Oct 09 23:06:49 2010\, sprout wrote:
On Sun Sep 05 13:15:28 2010\, sprout wrote:
That patch can cause a bus error\, so here is a new version. The third patch no longer applies with this version\, so here is a new version of that\, too (3b).
3b has been applied as 1c93aaac75354eeef.
Er\, actually 3e79609f389.
On Sun Sep 05 13:18:02 2010\, sprout wrote:
I plan to work on assignment to empty stash elements next (or any stash elements that are not globs). I can do this by using PVLVs. But there are two approaches:
1. Do it just for access from Perl by adding it to pp_helem. 2. Do it for XS access as well\, by modifying hv_fetch_ent instead (or hv_common\, or wherever the actual code is).
Number 1 makes it too easy for XS code to make changes without triggering mro_package_moved. mro_package_moved will also have to be made public (and probably given a better name). Number 2 is probably more reliable. perlâs internals can pass a flag to avoid the PVLVs. XS code that assumes that stash elements are never magical will be broken (code that specifically calls the _nomg forms to avoid the magic checks). Most XS code will continue to work.
So in both cases\, XS code will have to be modified (if there is any that plays with stashes). But in case 2 only code that is currently potentially buggy will have to change.
I prefer number 2.
Here is a patch for this. I used approach number 1 because number 2 proved to be far more complicated than I had thought\, and probably too risky. I am not so sure now that mro_package_moved needs to be made public. But we could change that later if necessary.
That fourth patch is no good. Not only does it have mistakes\, but the whole concept is flawed. Here are some of the problems:
⢠It only works with top-level packages ⢠References to stash elements that are not globs no longer reference the SVs themselves\, but the named stash elements. ⢠Actually\, those references would probably stringify with LVALUE\, not GLOB/SCALAR. ⢠Two references to the same element would not compare equal. ⢠References to stash elements containing glob copies--well\, let::s not get into that.
And I now think that assignments to empty stash elements should not be done by user code anyway. See #78074.
On Sun Sep 05 13:18:02 2010\, sprout wrote:
I plan to work on assignment to empty stash elements next (or any stash elements that are not globs). I can do this by using PVLVs. But there are two approaches:
1. Do it just for access from Perl by adding it to pp_helem. 2. Do it for XS access as well\, by modifying hv_fetch_ent instead (or hv_common\, or wherever the actual code is).
Number 1 makes it too easy for XS code to make changes without triggering mro_package_moved. mro_package_moved will also have to be made public (and probably given a better name). Number 2 is probably more reliable. perlâs internals can pass a flag to avoid the PVLVs. XS code that assumes that stash elements are never magical will be broken (code that specifically calls the _nomg forms to avoid the magic checks). Most XS code will continue to work.
So in both cases\, XS code will have to be modified (if there is any that plays with stashes). But in case 2 only code that is currently potentially buggy will have to change.
I prefer number 2.
Here is a patch for this. I used approach number 1 because number 2 proved to be far more complicated than I had thought\, and probably too risky. I am not so sure now that mro_package_moved needs to be made public. But we could change that later if necessary.
That fourth patch is no good. Not only does it have mistakes\, but the whole concept is flawed. Here are some of the problems:
⢠It only works with top-level packages ⢠References to stash elements that are not globs no longer reference the SVs themselves\, but the named stash elements. ⢠Actually\, those references would probably stringify with LVALUE\, not GLOB/SCALAR. ⢠Two references to the same element would not compare equal. ⢠References to stash elements containing glob copies--well\, let::s not get into that.
And I now think that assignments to empty stash elements should not be done by user code anyway. See #78074.
Now fixed by 80ebaca and a couple dozen patches leading up to it.
@cpansprout - Status changed from 'open' to 'resolved'
Migrated from rt.perl.org#75176 (status was 'resolved')
Searchable as RT75176$