Front page | perl.perl5.porters |
Postings from November 2016
Re: A possible new approach to COW - COW_META.
Thread Previous
|
Thread Next
From:
demerphq
Date:
November 7, 2016 08:40
Subject:
Re: A possible new approach to COW - COW_META.
Message ID:
CANgJU+Ut3m8ATVzNjKj1KRDtaTjYv0x=_M=GvL=Z9f3KKCj6Kw@mail.gmail.com
On 6 November 2016 at 21:50, Father Chrysostomos <sprout@cpan.org> wrote:
>
> On Nov 6, 2016, at 7:13 AM, demerphq <demerphq@gmail.com> wrote:
>> For now...
>> Take a look at the new branch and see what you think. As a patch
>> sequence it is a little raw,
>
> My perennial complaint: Unnecessary whitespace changes make it harder to see the actual code changes (and harder to blame, etc.).
Sorry about that. Some of the code involved was double indented which
made it hard to follow especially during debugging. I will try to
split out those changes, or not include them in a rebase of the
current branch.
> PerlIO::encoding:
> - SvLEN_set(e->dataSV, 0); /* Hands off sv.c - it isn't yours */
> + SvLEN_set(e->dataSV, 0); /* Hands off sv.c - it isn't yours (XXX: PERL_ANY_COW)*/
>
> I don’t understand that comment.
I added the PERL_ANY_COW because I had a bug where I *was* COW'ing
these buffers, and I couldnt figure out where they came from. After a
half day I managed to figure it out, and added the PERL_ANY_COW so
that if someone searched for it they would find that line.
Do you happen to know where the LEN=0 convention is documented?
>I non-COW string with SvLEN==0 has a ‘static’ buffer that sv_clear should not free. Simple enough.
Yes indeed.
>A cowy SvLEN==0 is a shared hash key.
This is something I would like to change, as it introduces unnecessary
complication in the COW codepaths. I have a patch that makes PL_strtab
use COW_META structures for its refcounts, and that then treats keys
the same way (except for destruction) as normal strings. The net
result once completed is to lose all the len == 0 checks in the COW
codepaths when SvIsCOW() is true.
> hv.c:
> + /* how does the following line work when the entry holds an SV?!! */
> if (memNE(HeKEY(entry),key,klen)) /* is this it? */
>
> SVs only come into play for tied hashes, if I understand correctly.
Sorry, that comment should not have been included in the patch. I came
to the same conclusion as you did, and meant to remove or change the
comment I added.
>
> op.c:ck_svconst:
> - if (!SvREADONLY(sv) && !SvIsCOW(sv) && SvCANCOW(sv)) {
> - SvIsCOW_on(sv);
> - CowREFCNT(sv) = 0;
> + if (
> + !SvREADONLY(sv) &&
> + !SvIsCOW(sv) &&
> + SvCANCOW(sv)
> + ) {
> + sv_cow_meta_setup(sv);
>
> Based on your description, that code can just be deleted now. The COW setup should not be necessary here.
Oh? Ok. Thanks. I will review in more detail
> newCOW_META (rather than new_COW_META) would match existing conventions more closely.
Ok, fair enough. Except its new_HE() and del_HE() which is where
cribbed the code. (note the differing definitions in sv.c and hv.c).
> sv.c:sv_setsv_flags:
> + : ( len /* XXX PERL_ANY_COW
> + len == 0 means private buffer which
> + for we will assume we cannot cow.
> + see ext/PerlIO-encoding/encoding.xs */
>
> s/XXX |for //g;
>
> In case you meant ‘for now’, I think this is a permanent state of affairs and should remain that way. A ‘static’ PV in an SV is a very useful feature. But it may not be truly static, so we cannot do COW with it, as some XS module might free the buffer while we are still hanging on to it.
FWIW, that statement doesn't entirely make sense to me. I understand
what you are getting at, but I dont see why we would have that trouble
any more with a COW SvPV than we would with a normal SvPV. Is there
something that I am overlooking?
> sv.c:sv_setsv_cow:
>
> + /* NEW COW XXX - why mutable, why sv_buf_to_ro? */
> new_pv = SvPVX_mutable(sstr);
> sv_buf_to_ro(sstr);
>
> Why mutable? Ask Nicholas. I’ve never really understood why we need a distinction between SvPVX_const and SvPVX_mutable. That’s not my line of code.
Ok, fair enough.
> We need sv_buf_to_ro here if we turned it off to increment the reference count. It can be deleted, as long as the sv_buf_to_rw a few lines earlier is also deleted.
Thanks, that was something I had meant to ask about but forgot. Under
this scheme COW can leave the buffers ro or rw status alone right? Or
do you think a COW buffer should always be marked as ro?
> sv.c:S_sv_uncow:
>
> + sv_buf_to_rw(sv); /* NOOP if RO-ing not supported *//* XXX: is this require anymore */
> + if (cm->cm_refcnt) {
> + cm->cm_refcnt--;
> + sv_buf_to_ro(sv);
> + } else {
> + del_COW_META(cm);
> + }
>
> sv_buf_to_rw is required if the SV is not going to be COW after this. It should go in the else.
Understood. Thanks.
> These comments are just from a quick skim; I have not tested or confirmed any of this. If the whitespace is screwy, it’s because I copied and pasted from gitweb.
>
> That’s all I have time for.
I really appreciate the time you took. Its great feedback.
>> but I wanted to push it out to a wider
>> audience so people could take a look.
>>
>> Thanks...
>> To FC for getting it working in the first place, changing it once it
>> works is a lot easier than doing it in the first place.
>
> I’m happy that I got the ball rolling. Thank you for continuing.
As they say, we all stand on the shoulders of giants. FWIW, I hope my
ankles arent digging into your neck. ;-)
Yves
--
perl -Mre=debug -e "/just|another|perl|hacker/"
Thread Previous
|
Thread Next