From zefram@fysh.org

src/main.nqp features this code​:

  # Bump up recursion limit, for VMs that have one.
  $comp.recursion_limit(100000);

The code and comment do not match​: the comment says that the recursion
limit is being raised, but the code unconditionally *sets* the limit.
If the VM defaults to a recursion limit greater than 100000, or to no
limit, and then honours the requested limit of 100000, the effect of
the code is to *reduce* the limit, contrary to the comment.

The only way the code and comment could be interpreted as matching is
if every VM with a settable recursion limit were obliged to default it
to less than 100000. Currently Parrot does have such a low default
limit, and the JVM doesn't have any settable limit, so both actually
satisfy this. But there doesn't seem to be any such requirement in the
Perl6​::Compiler API, so a future backend would presumably be free to
do otherwise.

The trivial way to make the code consistent is to change the comment​:

  # Set recursion limit, for VMs that have one.
  $comp.recursion_limit(100000);

The other way is to change the code. Any code change that better matches
the comment requires reading the recursion limit, which is not currently
part of the Perl6​::Compiler API. Independent of which way this particular
issue is resolved, if there's any support for setting a recursion limit
there should probably be support for reading it too.

Presuming such an addition to the API, the code can be made to match
the comment by unconditionally increasing the limit (modulo infinity
and overflow)​:

  # Bump up recursion limit, for VMs that have one.
  $comp.recursion_limit($comp.recursion_limit + 100000);

What I suspect is the real intent of this code doesn't match either the
comment or the code​:

  # Bump up recursion limit, for VMs that default to a low limit.
  $comp.recursion_limit(100000) if $comp.recursion_limit < 100000;

-zefram

From @coke

On Thu Nov 07 06​:20​:28 2013, zefram@​fysh.org wrote​:

src/main.nqp features this code​:

\# Bump up recursion limit, for VMs that have one\.
$comp\.recursion\_limit\(100000\);

The code and comment do not match​: the comment says that the recursion
limit is being raised, but the code unconditionally *sets* the limit.
If the VM defaults to a recursion limit greater than 100000, or to no
limit, and then honours the requested limit of 100000, the effect of
the code is to *reduce* the limit, contrary to the comment.

The only way the code and comment could be interpreted as matching is
if every VM with a settable recursion limit were obliged to default it
to less than 100000. Currently Parrot does have such a low default
limit, and the JVM doesn't have any settable limit, so both actually
satisfy this. But there doesn't seem to be any such requirement in the
Perl6​::Compiler API, so a future backend would presumably be free to
do otherwise.

The trivial way to make the code consistent is to change the comment​:

\# Set recursion limit, for VMs that have one\.
$comp\.recursion\_limit\(100000\);

The other way is to change the code. Any code change that better matches
the comment requires reading the recursion limit, which is not currently
part of the Perl6​::Compiler API. Independent of which way this particular
issue is resolved, if there's any support for setting a recursion limit
there should probably be support for reading it too.

Presuming such an addition to the API, the code can be made to match
the comment by unconditionally increasing the limit (modulo infinity
and overflow)​:

\# Bump up recursion limit, for VMs that have one\.
$comp\.recursion\_limit\($comp\.recursion\_limit \+ 100000\);

What I suspect is the real intent of this code doesn't match either the
comment or the code​:

\# Bump up recursion limit, for VMs that default to a low limit\.
$comp\.recursion\_limit\(100000\) if $comp\.recursion\_limit \< 100000;

-zefram

This is only used for parrot - it's a noop on the other 2 nqp backends - I recommend wrapping this line in a #? preprocessor block for parrot only, and changing the comment to reflect that we're just doing this for a single vm.

--
Will "Coke" Coleda

From @pmichaud

On Thu, Nov 14, 2013 at 06​:13​:39AM -0800, Will Coleda via RT wrote​:

This is only used for parrot - it's a noop on the other 2 nqp backends.
I recommend wrapping this line in a #? preprocessor block for parrot
only, and changing the comment to reflect that we're just doing this
for a single vm.

In general I greatly prefer to reduce the number of #? preprocessing
blocks, not increase them. In this case I'd prefer to simply change
the comment to reflect what's really happening and then close this
ticket.

Even though we don't have a recursion limit for other backends, I
can greatly imagine there would be some utility to adding one at
some point in the future.

Pm

From @diakopter

"Even though we don't have a recursion limit for other backends, I can
greatly imagine there would be some utility to adding one at some point in
the future."

+1

On Thu, Nov 14, 2013 at 6​:34 AM, Patrick R. Michaud <pmichaud@​pobox.com>wrote​:

On Thu, Nov 14, 2013 at 06​:13​:39AM -0800, Will Coleda via RT wrote​:

This is only used for parrot - it's a noop on the other 2 nqp backends.
I recommend wrapping this line in a #? preprocessor block for parrot
only, and changing the comment to reflect that we're just doing this
for a single vm.

In general I greatly prefer to reduce the number of #? preprocessing
blocks, not increase them. In this case I'd prefer to simply change
the comment to reflect what's really happening and then close this
ticket.

Even though we don't have a recursion limit for other backends, I
can greatly imagine there would be some utility to adding one at
some point in the future.

Pm

--
Sent by the Internet

Login to LinkedIn to see my whole profile and Connect
http://linkedin.com/in/mattswilson

@coke - Status changed from 'new' to 'open'

From @coke

On Thu Nov 14 06​:34​:57 2013, pmichaud wrote​:

On Thu, Nov 14, 2013 at 06​:13​:39AM -0800, Will Coleda via RT wrote​:

This is only used for parrot - it's a noop on the other 2 nqp backends.
I recommend wrapping this line in a #? preprocessor block for parrot
only, and changing the comment to reflect that we're just doing this
for a single vm.

In general I greatly prefer to reduce the number of #? preprocessing
blocks, not increase them. In this case I'd prefer to simply change
the comment to reflect what's really happening and then close this
ticket.

While I agree in general that we should be hiding these differences inside ops in nqp, I disagree on this particular one.

It's dealing with a specific behavior needed of the VM by rakudo which isn't appropriate to push into nqp (not every thing using NQP needs the same recursion limit of the lower level VM, e.g.).

Even though we don't have a recursion limit for other backends, I
can greatly imagine there would be some utility to adding one at
some point in the future.

At which point the limit on each backend will no doubt need to be different, and we'll need the conditional code even more.

Pm

--
Will "Coke" Coleda

From zefram@fysh.org

Will Coleda via RT wrote​:

It's dealing with a specific behavior needed of the VM by rakudo which
isn't appropriate to push into nqp
...
At which point the limit on each backend will no doubt need to be
different,

Interesting​: your view here implies a rather different intent to the
recursion_limit code than any of the options I considered. I'd supposed
that the intent was to provide a settable recursion limit where possible,
looking essentially similar to HLL code across VMs, with only the
implementation differing. But if essentially different behaviour is
wanted per VM, that sounds more like you're intending to cope with
different kinds of limit imposed by each VM, hiding it entirely from
HLL code.

If the intent is to cope with, and hide, the fact that Parrot imposes a
(low) recursion limit by default, the value you should set the limit to
is -1. That is, -1 as it appears in the interpreter recursion_limit
method that takes a Parrot int (signed word) for its parameter.
Internally the recursion limit is interpreted as an *un*signed word,
so the -1 becomes the highest possible value. In fact, since the
comparison (in src/pmc/sub.pmc) is of the form depth>limit, with the
limit at all-bits-one the exception can never be triggered even if the
depth count somehow becomes decoupled from actual stack frame records
and grows enough to wrap around.

-zefram

From @coke

On Sun Dec 08 17​:40​:36 2013, zefram@​fysh.org wrote​:

Will Coleda via RT wrote​:

It's dealing with a specific behavior needed of the VM by rakudo which
isn't appropriate to push into nqp
...
At which point the limit on each backend will no doubt need to be
different,

Interesting​: your view here implies a rather different intent to the
recursion_limit code than any of the options I considered. I'd supposed
that the intent was to provide a settable recursion limit where possible,
looking essentially similar to HLL code across VMs, with only the
implementation differing. But if essentially different behaviour is
wanted per VM, that sounds more like you're intending to cope with
different kinds of limit imposed by each VM, hiding it entirely from
HLL code.

If the intent is to cope with, and hide, the fact that Parrot imposes a
(low) recursion limit by default, the value you should set the limit to
is -1. That is, -1 as it appears in the interpreter recursion_limit
method that takes a Parrot int (signed word) for its parameter.
Internally the recursion limit is interpreted as an *un*signed word,
so the -1 becomes the highest possible value. In fact, since the
comparison (in src/pmc/sub.pmc) is of the form depth>limit, with the
limit at all-bits-one the exception can never be triggered even if the
depth count somehow becomes decoupled from actual stack frame records
and grows enough to wrap around.

-zefram

Rakudo no longer targets the parrot backend of NQP - I've removed the code in rakudo that set the recursion limit since it's a noop on the backends we do target, closing this ticket.

The code to attempt to set the recursion limit still exists in NQP - If someone feels strongly about cleaning up NQP, a PR can be opened at https://github.com/perl6/nqp - be sure to include a reference back to this ticket.

--
Will "Coke" Coleda

@coke - Status changed from 'open' to 'resolved'