Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.85k stars 524 forks source link

Perl_op_convert_list: Avoid unnecessary processing of CONST OPs #22116

Open richardleach opened 4 weeks ago

richardleach commented 4 weeks ago

When _Perl_op_convertlist is called to stringify a CONST OP, a fair amount of activity occurs in order to return either an unchanged (if called without the OPf_FOLDED flag) or only slightly changed (with OPf_FOLDED) OP.

In these commits, any changes are done directly and the CONST OP returned without wrapping it in a new list OP and constant-folding it back again.

Please note: This almost certainly should be a defer-next-dev PR!

tonycoz commented 4 weeks ago

The change looks reasonable, but the added folding code doesn't execute much, from coverage testing against make test_harness:

 13511958: 5535:    if (type == OP_STRINGIFY && OP_TYPE_IS(o, OP_CONST) ) {
        -: 5536:        /* Don't wrap a single CONST in a list, process that list,
        -: 5537:         * then constant fold the list back to the starting OP. */
        -: 5538:        assert(!OpSIBLING(o));
        -: 5539:
  2208678: 5540:        if (flags & OPf_FOLDED) {
      167: 5541:            o->op_folded = 1;
      167: 5542:            SV *sv = cSVOPx_sv(o);
        -: 5543:
      167: 5544:            if (SvPOK(sv))
      165: 5545:                SvFLAGS(sv) |= SVs_PADTMP;
        -: 5546:            else {
        2: 5547:                SV *newsv = newSV_type(SVt_PV);
        2: 5548:                SvFLAGS(newsv) |= SVs_PADTMP;
        2: 5549:                sv_copypv_nomg(newsv, sv);
        2: 5550:                SvFLAGS(newsv) |= (SVf_READONLY|SVf_PROTECT);
        2: 5551:                SvREFCNT_dec_NN(sv);
        -: 5552:#ifdef USE_ITHREADS
        -: 5553:                if (cSVOPx(o)->op_sv) {
        -: 5554:                    cSVOPx(o)->op_sv = newsv;
        -: 5555:                } else {
        -: 5556:                    PAD_SVl((o)->op_targ) = newsv;
        -: 5557:                }
        -: 5558:#else
        2: 5559:                cSVOPx(o)->op_sv = newsv;
        -: 5560:#endif
        -: 5561:            }
        -: 5562:        }
  2208678: 5563:        return o;
        -: 5564:
        -: 5565:    }
 11303280: 5566:    if (!o || o->op_type != OP_LIST)
richardleach commented 4 weeks ago

Thanks for looking at this, @tonycoz.

but the added folding code doesn't execute much

In that case, I'll remove the folding code and add in an extra comment about its absence.

How did you generate that coverage report please?

richardleach commented 4 weeks ago

I'll remove the folding code and add in an extra comment about its absence.

Now done.

tonycoz commented 4 weeks ago

How did you generate that coverage report please?

I built perl following the instructions under "GCC gcov profiling" in perlhacktips, then: