Q: re Inline and Benchmark

[Michele Dondi]

While revising the benchmarks I had done for the fast random string
generation thread of a few weeks ago I wanted to include Abigail's
Inline::C solution too. Now, in the original ones I gave cmpthese() a
second param of the form:

{
...
Solution => \&Solution,
...
};

But this *doesn't* work[1] with the Inline::C solution, even if I
predeclare the sub. I've succeeded with

Inline => sub { Inline() },

and BTW it still largely outperforms all the other ones, but I'm
bothered by the fact that it involves one extra sub call which IMHO is
not elegant at all and, even if only marginally relevant, not fair for
comparison purposes. So, do you have any suggestion on what I should
do instead?

Also, the previous question was not terribly specific of Benchmark.pm
(but only of the kind of parameters one of its functions wants), so
more on-topic: is there any caveat with benchmarks carried on
Inline::C subs? I don't think so, but just to be sure...

As a side note, since I want to simply cmpthese() a bunch of (named)
subs giving as labels their respective names, I don't want to write
things twice as above and so was thinking of using a map() instead.
Now I'm unsure which of these would be better:

{ map { $_ => "$_()" } qw/sol1 sol2 .../ };

{ map {
no strict 'refs';
$_ => \&{$_};
} qw/sol1 sol2 .../ };

Generally speaking, I don't particularly like the 'code' version, and
I suspect it to be less efficient too[2]. (Well, I just tried: it is!)
Well, any advice?

[1] For some IMHO sensible acceptation of "doesn't work", in the sense
that I've tried some quite obvious variations, but then I'd be happy
to know that I was just too dumb and missed somethins obvious.

[2] Not that this is terribly relevant, but since we're speaking of
something that must be repeated *many* times, anyway...


Michele

 

[Sisyphus]

use Inline C => <<'--';
char * r_string () {
char * str;
int i;
int l;

l = 20000;
srand (time ((time_t) NULL));
if ((str = (char *) malloc (l * sizeof (char))) == (char *) NULL) {
perror (malloc);
exit (-1);
}
for (i = 0; i < l; i ++) {
str = rand () % 255;
}
return (str);
}

The above will leak memory because 'str' never gets freed.
One alternative (that doesn't leak) is to have 'r_string' return a 'SV
*' rather than a 'char *'. (I would also use 'New' and 'Safefree' in
place of 'malloc' and 'free'. Not sure about 'perror' either, so I'll
opt for 'croak' - with which I *am* familiar

Something like (untested):

SV * r_string () {
char * str;
SV * outsv;
int i;
int l;

l = 20000;
srand (time ((time_t) NULL));
New(1, str, l * sizeof(char), char);
if(str == NULL)
croak("Failed to allocate memory in r_string()");
for (i = 0; i < l; i ++) {
str = rand () % 255;
}
outsv = newSVpv(outstr, l)
Safefree(str);
return outsv;
}


'str' now gets freed inside the subroutine, and perl takes care of the
SV* that has been returned.

Cheers,
Rob

 

[Sisyphus]

Also, the previous question was not terribly specific of Benchmark.pm
(but only of the kind of parameters one of its functions wants), so
more on-topic: is there any caveat with benchmarks carried on
Inline::C subs? I don't think so, but just to be sure...

I've benchmarked hundreds of Inline::C subs and never struck any.

Cheers,
Rob

 

[Tassilo v. Parseval]

Also sprach Sisyphus:

use Inline C => <<'--';
char * r_string () {
char * str;
int i;
int l;

l = 20000;
srand (time ((time_t) NULL));
if ((str = (char *) malloc (l * sizeof (char))) == (char *) NULL) {
perror (malloc);
exit (-1);
}
for (i = 0; i < l; i ++) {
str = rand () % 255;
}
return (str);
}

The above will leak memory because 'str' never gets freed.
One alternative (that doesn't leak) is to have 'r_string' return a 'SV
*' rather than a 'char *'. (I would also use 'New' and 'Safefree' in
place of 'malloc' and 'free'. Not sure about 'perror' either, so I'll
opt for 'croak' - with which I *am* familiar

Something like (untested):

SV * r_string () {
char * str;
SV * outsv;
int i;
int l;

l = 20000;
srand (time ((time_t) NULL));
New(1, str, l * sizeof(char), char);

No need for the sizeof. New's fourth paramater takes care of that:

New(1, str, l, char);

if(str == NULL)
croak("Failed to allocate memory in r_string()");
for (i = 0; i < l; i ++) {
str = rand () % 255;
}
outsv = newSVpv(outstr, l)
Safefree(str);
return outsv;

That will leak, too. 'outsv' must be made mortal in order to get freed on
block boundaries:

return sv_2mortal(outsv);

}


'str' now gets freed inside the subroutine, and perl takes care of the
SV* that has been returned.

Not quite.

Allocating a string, then copy it into an SV after which the string is
freed is also a bit wasteful. A better solution:

New(1, str, l, char);
...
outsv = sv_newmortal();
sv_usepvn(outsv, str, l);
return outsv;

Perl will now use the memory of 'str' for 'outsv' and no double
allocation is done.

Tassilo

 

[Sisyphus]

Also sprach Sisyphus:

Abigail wrote:

use Inline C => <<'--';
char * r_string () {
char * str;
int i;
int l;

l = 20000;
srand (time ((time_t) NULL));
if ((str = (char *) malloc (l * sizeof (char))) == (char *) NULL) {
perror (malloc);
exit (-1);
}
for (i = 0; i < l; i ++) {
str = rand () % 255;
}
return (str);
}

The above will leak memory because 'str' never gets freed.
One alternative (that doesn't leak) is to have 'r_string' return a 'SV
*' rather than a 'char *'. (I would also use 'New' and 'Safefree' in
place of 'malloc' and 'free'. Not sure about 'perror' either, so I'll
opt for 'croak' - with which I *am* familiar

Something like (untested):

SV * r_string () {
char * str;
SV * outsv;
int i;
int l;

l = 20000;
srand (time ((time_t) NULL));
New(1, str, l * sizeof(char), char);

No need for the sizeof. New's fourth paramater takes care of that:

New(1, str, l, char);

if(str == NULL)
croak("Failed to allocate memory in r_string()");
for (i = 0; i < l; i ++) {
str = rand () % 255;
}
outsv = newSVpv(outstr, l)
Safefree(str);
return outsv;

That will leak, too. 'outsv' must be made mortal in order to get freed on
block boundaries:

return sv_2mortal(outsv);

I don't think so - and messrs Jenness and Cozens agree with me (page 241
of Extending and Embedding Perl). I think we touched on this in an XS
list discussion some time back and decided that mortalisation was not
necessary in an Inline C environment .... should I check on that? .... I
seem to recall that Nick stated that the C code revealed that the return
value *was* mortalised. Certainly it won't hurt to mortalise that code
as you have done - but I'm fairly sure it's not necessary .... I've got
some code to rewrite if I'm wrong

Not quite

Allocating a string, then copy it into an SV after which the string is
freed is also a bit wasteful. A better solution:

New(1, str, l, char);
...
outsv = sv_newmortal();
sv_usepvn(outsv, str, l);
return outsv;

Yes - I was going to take that approach but didn't want to get into
presenting code that was going to set each character within the SV
individually (for simplicity). Much easier (for me, anyway) to simply
leave the for() loop as is, and copy the result into an SV.

Cheers,
Rob

 

[Sisyphus]

I don't think so - and messrs Jenness and Cozens agree with me (page 241
of Extending and Embedding Perl). I think we touched on this in an XS
list discussion some time back and decided that mortalisation was not
necessary in an Inline C environment .... should I check on that? .... I
seem to recall that Nick stated that the C code revealed that the return
value *was* mortalised. Certainly it won't hurt to mortalise that code
as you have done - but I'm fairly sure it's not necessary .... I've got
some code to rewrite if I'm wrong

I don't have to worry - it's as I thought. The C code reveals that
returning an SV as I did (in Inline C) pushes the SV onto the stack,
where it is made mortal and then returned to perl. My code does *not*
leak memory and returning an 'sv_2mortal' in this case is simply redundant.

Cheers,
Rob

 

[Tassilo v. Parseval]

Also sprach Sisyphus:

Tassilo v. Parseval wrote:

I don't think so - and messrs Jenness and Cozens agree with me (page 241
of Extending and Embedding Perl). I think we touched on this in an XS
list discussion some time back and decided that mortalisation was not
necessary in an Inline C environment .... should I check on that? .... I
seem to recall that Nick stated that the C code revealed that the return
value *was* mortalised. Certainly it won't hurt to mortalise that code
as you have done - but I'm fairly sure it's not necessary .... I've got
some code to rewrite if I'm wrong

You seem to be right. According to Devel:eek, it is mortalized even
when it's not. As it looks this is not Inline::C specific but applies to
XS as well. The xsubpp utitily adds 'sv_2mortal(ST($num))' before it's
XSRETURNing. I find this a little bit annoying because that is usually
the thing typemaps are for. I hate such (possibly undocumented)
smartery.

As for not hurting: Actually it does hurt when you mortalize here. You
get an "Attempt to free unreferenced scalar" warning if you do.

Tassilo

 

[Sisyphus]

Also sprach Sisyphus:




You seem to be right. According to Devel:eek, it is mortalized even
when it's not. As it looks this is not Inline::C specific but applies to
XS as well. The xsubpp utitily adds 'sv_2mortal(ST($num))' before it's
XSRETURNing. I find this a little bit annoying because that is usually
the thing typemaps are for. I hate such (possibly undocumented)
smartery.

Oh ... I assumed it was an Inline::C thing. I guess that if you don't
want to mortalise it then, instead of 'return'ing it, you would push it
onto the stack as a non-mortal. That way it won't get to be mortalised.
At least that's what you'd do in Inline::C:

Inline_Stack_Vars;
..
..
Inline_Stack_Push(outsv);
Inline_Stack_Done;
Inline_Stack_Return(1);

Now 'outsv' has been returned to perl without mortalising it. (I don't
know the XS equivalent to that.)

As for not hurting: Actually it does hurt when you mortalize here. You
get an "Attempt to free unreferenced scalar" warning if you do.

Yes - I just found that myself a few minutes ago. It's not so "simply
redundant" (as I subsequently posted) after all

Cheers,
Rob

 

[Michele Dondi]

I've benchmarked hundreds of Inline::C subs and never struck any.

So, since you've been so gentle answering this particular question of
mine, could you please be so kind and answer the other ones too? I am
aware that my OP was probably too verbose...


Michele

 

[Michele Dondi]

use Inline C => <<'--';

[snip code]

cmpthese 500 => {
C => 'r_string',
Loop1 => \&Loop1,
Loop2 => \&Loop2,
...
}

TY! However... I apologize for my fussiness, but would this be fair?
Some quick tests showed that, in a general situation, there are
different performance responses with coderefs and strings: I think
that in one case dereferencing and sub calls are involved whereas in
the other an eval() takes place:

# perl -MBenchmark=:all -e 'sub a { $|++ }\
cmpthese 20_000_000, { str => "a", code => \&a }'
Rate str code
str 922084/s -- -46%
code 1693480/s 84% --

I don't know if things are different with Inline subs, but I doubt
that...


Michele

 

[Ilya Zakharevich]

[A complimentary Cc of this posting was sent to
Tassilo v. Parseval

That will leak, too. 'outsv' must be made mortal in order to get freed on
block boundaries:

^^^^^^^^^^^^^^^^

Actually, "Beginning of the next statement".

Allocating a string, then copy it into an SV after which the string is
freed is also a bit wasteful. A better solution:

New(1, str, l, char);
...
outsv = sv_newmortal();
sv_usepvn(outsv, str, l);
return outsv;

This is also wasteful, since usepvn() calls realloc() to add \0 at the
end. I would use something like

outsv = sv_newmortal();
SvGROW(outsv, l);
str = SvPV_nolen(outsv);
...
return outsv;

Hope this helps,
Ilya

 

[Ilya Zakharevich]

[A complimentary Cc of this posting was sent to
Sisyphus

I don't have to worry - it's as I thought. The C code reveals that
returning an SV as I did (in Inline C) pushes the SV onto the stack,
where it is made mortal and then returned to perl. My code does *not*
leak memory and returning an 'sv_2mortal' in this case is simply redundant.

sv_mortal() can never be "redundant". It must be called *exactly* a
particular number of times (just like free()): if you call it less,
you get a memory leak. If you call it more, you get a crash soon.

Hope this helps,
Ilya

P.S. Probably you mean "approach of replacing char* with a mortal
SV*" is redundant...

 

[Tassilo v. Parseval]

Also sprach Ilya Zakharevich:

Tassilo v. Parseval


This is also wasteful, since usepvn() calls realloc() to add \0 at the
end.

Bah, true. It will do that even when the string is already null-padded.

I would use something like

outsv = sv_newmortal();
SvGROW(outsv, l);
str = SvPV_nolen(outsv);
...
return outsv;

Since SvGROW will upgrade a fresh SV to a PV, one could get away with
just using SvPVX and thus save the ternary operation in SvPV_nolen. And
again half a nanosecond is gained. ;-)

Tassilo

 

[Sisyphus]

sv_mortal() can never be "redundant". It must be called *exactly* a
particular number of times (just like free()): if you call it less,
you get a memory leak. If you call it more, you get a crash soon.

Hope this helps,
Ilya

P.S. Probably you mean "approach of replacing char* with a mortal
SV*" is redundant...

No ... I meant what I said ... and, yes, I can be *that* dumb. When I
wrote that, I wasn't paying attention to what I was saying

I think I was preoccupied with the "memory leak" accusation to the
exclusion of all other sensible thought - and I was in a hurry.

It wasn't until a few minutes later that I got around to thinking about
it, and actually ran some code to test it out ... and then Tassilo's
post correcting me appeared shortly after.

Cheers,
Rob

 

[Michele Dondi]

-: > cmpthese 500 => {
-: > C => 'r_string',
-: > Loop1 => \&Loop1, [snip]
Indeed. So, \&Loop1 calls one sub per iteration (the sub "Loop1"), and
'r_string' calls one sub per iteration (the sub "r_string").

I did it this way to be sure it's fair - all the benchmarks now call
the same number of subs.

That was clear enough. What I'm bothered about is that different
methods by which these subs are called may introduce different
overheads. Now, this bias may become negligible as soon as sub
execution times get long enough, but I'm not sure about how much that
"enough" could be.

In other words, IMHO a fair comparison would be that between

C => 'r_string',
Loop1 => 'Loop1',

and if needed I will stick to that. However, more generally speaking,
is there a way I can take a reference of an Inline sub?


Michele

 

[Anno Siegel]

[...]

and if needed I will stick to that. However, more generally speaking,
is there a way I can take a reference of an Inline sub?

It's in no way different from a normal sub: "\&foo".

Anno

 

[Sisyphus]

[...]

and if needed I will stick to that. However, more generally speaking,
is there a way I can take a reference of an Inline sub?

It's in no way different from a normal sub: "\&foo".

Hmmm ... that's not what I'm finding.

If I do \&foo all I get is a "Usage:" warning telling me that it needs
to be foo() - and when I try \&foo() I get a slightly different "usage:"
warning telling me (in effect) that I need to use 'timethis' (and *no*
sub reference) instead of 'cmpthese' (with a sub reference).

Seems to me that Inline subs are not like normal subs in that, although
they belong to package "main", they belong to (something like) module
"scriptname_pl_****" where the * stands for a hex digit.

I'm thinking maybe Michele's question amounts to "how do I take a
reference of a loaded module's sub?" (in the context of Benchmark's
'cmpthese'). I don't know the answer to that.

Cheers,
Rob

 

[Sisyphus]

So, since you've been so gentle answering this particular question of
mine, could you please be so kind and answer the other ones too? I am
aware that my OP was probably too verbose...

Well ... I didn't really understand the other questions (partly because
I hadn't followed the original thread) and I assumed that Abigail had
adequately answered them. All I really wanted to assure you of is that
"I've benchmarked hundreds of Inline::C subs and never struck any"
caveats

Of course, I've only ever used 'timethese()' ... and I'm now starting to
realise that the issue you're raising is in regard to 'cmpthese()' and
references to Inline subroutines. Mind you - no matter what - I'm going
to insist that any caveats are "Benchmark" caveats and not "Inline::C"
caveats

I've since taken a look at the original thread and noted that Abigail
posted the same (leaking) code there - and that no-one mentioned the
fact that it leaks. What I'm now wondering is whether the leak was not
mentioned because:
a) no-one noticed it;
or because:
b) code that leaks is of little concern, and generally not considered
worth mentioning.

(I hate to think that I'm guilty of nitpicking over some minor code
imperfection. If that's what I did, could someone please let me know, so
that I can be sure not to re-offend in the future.)

Cheers,
Rob

 

[Anno Siegel]

[...]

and if needed I will stick to that. However, more generally speaking,
is there a way I can take a reference of an Inline sub?

It's in no way different from a normal sub: "\&foo".

Hmmm ... that's not what I'm finding.

If I do \&foo all I get is a "Usage:" warning telling me that it needs
to be foo() - and when I try \&foo() I get a slightly different "usage:"
warning telling me (in effect) that I need to use 'timethis' (and *no*
sub reference) instead of 'cmpthese' (with a sub reference).

That's an artifact of Benchmark. Inline checks parameters and gives
the "usage" message you've seen when (for instance) a function requiring
no arguments is called with arguments. Benchmark calls a given
coderef in the form "&$subref;" (line 646 in v 1.051), so if @_ happens to
be non-empty at this point, the error occurs.

Prototyped Perl subs would have a similar problem. Time for a bug report,
I guess... And another lesson why one shouldn't call arbitrary subs
with &.

Anno

 

[Michele Dondi]

fact that it leaks. What I'm now wondering is whether the leak was not
mentioned because:
a) no-one noticed it;
or because:
b) code that leaks is of little concern, and generally not considered
worth mentioning.

Certainly *for me* and *in this case* it's not necessarily worth
mentioning that it leaks. However since I could understand Abigail's
code[1], but I can't follow your discussion anymore and yet I'd like
to include the non-leaking code in the comparison too (for
completeness) once you all agree on "the best/correct way to do it",
for some reasonably sensible meaning of "best/correct", may you supply
a complete, working, function? You can write me privately at
<ti tod nfni tod im tod mcl ta razalb> if needed.


[1] I've not written a line in C in ten years or so, but I conserve
some reminiscence of it. ;-)


Michele