EXTEND_MORTAL usage is questionable

[p5pRT]

Migrated from rt.perl.org#121845 (status was 'open')

Searchable as RT121845$

[p5pRT]

From @bulk88

Created by @bulk88

While looking through the core, I found usage of the EXTEND_MORTAL
macro. It seems to be used as an optimization for later sv_newmortal or
sv_2mortal calls. The problem is, EXTEND_MORTAL is executed AGAIN in
every sv_newmortal and sv_2mortal through PUSH_EXTEND_MORTAL__SV_C.

bbce6d6

-----------------------------------------------------------------
Revision​: bbce6d6
Author​: Perl 5 Porters <perl5-porters@​africa.nicoh.com>
Date​: 11/26/1996 3​:48​:00 AM
Message​:
[inseparable changes from patch from perl5.003_08 to perl5.003_09]
-----------------------------------------------------------------

later on in that commit

-----------------------------------------------------------------
Subject​: Mortal stack pre-allocation from Ilya.
From​: Chip Salzenberg <chip@​atlantic.net>
Files​: pp.c pp.h pp_ctl.c pp_hot.c pp_sys.c
-----------------------------------------------------------------
I can't find that msg ^, might before archiving of the ML so I guess the
original rational is lost. From some manual (might have missed one or 2)
git blamining all the EXTEND_MORTAL for a list/loop calls came from
bbce6d, in 1 case it came from a newer commit which has a lil rational

http​://perl5.git.perl.org/perl.git/commitdiff/cc5e57d2fd9821a8c93d8121736585ce8a41aa94
http​://markmail.org/thread/zq7x2ndkdvqodill

Something isn't fully thought out here. Either we need sv_newmortalnx
and sv_2mortalnx (nx=no extend), or EXTEND_MORTAL needs to be removed.
Perl_tmps_grow already overextends as of
http​://perl5.git.perl.org/perl.git/commitdiff/677b06e3672f6584546f6a391abaf54a138910e8
from 1999. Or 677b0's code needs to be partially reverted so tmps_grow
doesn't take a count anymore and always extends by 512 units as it was
in the past (but then no more non-fixed/loop/list context EXTEND_MORTAL
calls anymore). Another idea is to make a list/looping sv2mortal and
pass it a chunk of an AV or a chunk of Perl stack to mortalize. It will
remove alot of function calls when generating long fixed return lists
like in pp_stat.

_________________________________________________
void
Perl_sv2mortalarr(pTHX_ SV** svp, Size_t len) {
_________________________________________________

or
_________________________________________________
/* pass atleast 1 SV * to mortalize, at the end put a NULL to indicate
you finished your list of SV *s. */
void
Perl_sv2mortalva(pTHX_ SV * sv, ...) {
_________________________________________________

Something needs to be cleaned up, I'm not sure what.

Some interesting changes below

--------------------------------------------------------------
-STATIC void
-sv_mortalgrow(void)
-{
- dTHR;
- PL_tmps_max += (PL_tmps_max < 512) ? 128 : 512;
- Renew(PL_tmps_stack, PL_tmps_max, SV*);
-}
--------------------------------------------------------------

The 512 vs 128 thing came from
http​://perl5.git.perl.org/perl.git/commit/55497cffdd24c959994f9a8ddd56db8ce85e1c5b?f=sv.c
with no explination

----------------------------------------------------------------
@​@​ -3052,7 +3073,7 @​@​ register SV *sv;
  static void
  sv_mortalgrow()
  {
- tmps_max += 128;
+ tmps_max += (tmps_max < 512) ? 128 : 512;
  Renew(tmps_stack, tmps_max, SV*);
  }
----------------------------------------------------------------

128 number came from perl 5.0 alpha 6/~day 1 of 5.0

sv.c diff:
https://github.com/perl/perl5/compare/ed6116ce9b9d13712ea252ee248b0400653db7f9..8990e3071044a96302560bbdb5706f3e74cf1bef#diff-d237a07e2749aaf0dd44085eee9bd94b

Perl Info

Flags:
     category=core
     severity=low

Site configuration information for perl 5.19.12:

Configured by Owner at Tue Apr 29 12:19:05 2014.

Summary of my perl5 (revision 5 version 19 subversion 12) configuration:
   Derived from: 605728659d1010fcc2bbda42f494757fe6a6126e
   Ancestor: 9e9002efd1609c7d154f98af43a026320df7582c
   Platform:
     osname=MSWin32, osvers=5.1, archname=MSWin32-x86-multi-thread
     uname=''
     config_args='undef'
     hint=recommended, useposix=true, d_sigaction=undef
     useithreads=define, usemultiplicity=define
     use64bitint=undef, use64bitall=undef, uselongdouble=undef
     usemymalloc=n, bincompat5005=undef
   Compiler:
     cc='cl', ccflags ='-nologo -GF -W3 -O1 -MD -Zi -DNDEBUG -G7 -GL 
-DWIN32 -D_CONSOLE -DNO_STRICT  -DPERL_TEXTMODE_SCRIPTS 
-DPERL_HASH_FUNC_ONE_AT_A_TIME -DPERL_IMPLICIT_CONTEXT 
-DPERL_IMPLICIT_SYS -DUSE_PERLIO -D_USE_32BIT_TIME_T',
     optimize='-O1 -MD -Zi -DNDEBUG -G7 -GL',
     cppflags='-DWIN32'
     ccversion='13.10.6030', gccversion='', gccosandvers=''
     intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
     d_longlong=undef, longlongsize=8, d_longdbl=define, longdblsize=8
     ivtype='long', ivsize=4, nvtype='double', nvsize=8, 
Off_t='__int64', lseeksize=8
     alignbytes=8, prototype=define
   Linker and Libraries:
     ld='link', ldflags ='-nologo -nodefaultlib -debug -opt:ref,icf 
-ltcg  -libpath:"c:\perl519\lib\CORE"  -machine:x86'
     libpth="C:\Program Files\Microsoft Visual Studio .NET 2003\VC7\lib"
     libs=oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib 
comdlg32.lib advapi32.lib shell32.lib ole32.lib oleaut32.lib 
netapi32.lib uuid.lib ws2_32.lib mpr.lib winmm.lib  version.lib 
odbc32.lib odbccp32.lib comctl32.lib msvcrt.lib
     perllibs=oldnames.lib kernel32.lib user32.lib gdi32.lib 
winspool.lib  comdlg32.lib advapi32.lib shell32.lib ole32.lib 
oleaut32.lib  netapi32.lib uuid.lib ws2_32.lib mpr.lib winmm.lib 
version.lib odbc32.lib odbccp32.lib comctl32.lib msvcrt.lib
     libc=msvcrt.lib, so=dll, useshrplib=true, libperl=perl519.lib
     gnulibc_version=''
   Dynamic Linking:
     dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' '
     cccdlflags=' ', lddlflags='-dll -nologo -nodefaultlib -debug 
-opt:ref,icf -ltcg  -libpath:"c:\perl519\lib\CORE"  -machine:x86'

Locally applied patches:
     uncommitted-changes
     605728659d1010fcc2bbda42f494757fe6a6126e


@INC for perl 5.19.12:
     C:/perl519/site/lib
     C:/perl519/lib
     .


Environment for perl 5.19.12:
     HOME (unset)
     LANG (unset)
     LANGUAGE (unset)
     LD_LIBRARY_PATH (unset)
     LOGDIR (unset)
     PATH=C:\perl519\bin;C:\Program Files\Microsoft Visual Studio .NET 
2003\Common7\IDE;C:\Program Files\Microsoft Visual Studio .NET 
2003\VC7\BIN;C:\Program Files\Microsoft Visual Studio .NET 
2003\Common7\Tools;C:\Program Files\Microsoft Visual Studio .NET 
2003\Common7\Tools\bin\prerelease;C:\WINDOWS\system32;C:\WINDOWS;C:\WINDOWS\system32\wbem;
     PERL_BADLANG (unset)
     SHELL (unset)

[p5pRT]

From @tonycoz

On Sat May 10 21​:57​:01 2014, bulk88 wrote​:

While looking through the core, I found usage of the EXTEND_MORTAL
macro. It seems to be used as an optimization for later sv_newmortal
or
sv_2mortal calls. The problem is, EXTEND_MORTAL is executed AGAIN in
every sv_newmortal and sv_2mortal through PUSH_EXTEND_MORTAL__SV_C.

Most of the EXTEND_MORTAL() calls I see make sense in the case where the code might return a large number of mortals, like in pp_aassign and pp_mapwhile, since it will avoid expensive reallocations of PL_tmps_stack.

It's obviously much less useful in some other places​:

- pp_stat - it always returns 0 or 13 items, and 13 is well below the minimum growth size (128)

- pp_gmtime - always extends by 1 or 9.

Do any of the other uses strike you as wasteful or incorrect?

Tony

[p5pRT]

The RT System itself - Status changed from 'new' to 'open'

[p5pRT]

From @bulk88

On Tue Oct 07 20​:49​:00 2014, tonyc wrote​:

On Sat May 10 21​:57​:01 2014, bulk88 wrote​:

While looking through the core, I found usage of the EXTEND_MORTAL
macro. It seems to be used as an optimization for later sv_newmortal
or
sv_2mortal calls. The problem is, EXTEND_MORTAL is executed AGAIN in
every sv_newmortal and sv_2mortal through PUSH_EXTEND_MORTAL__SV_C.

Most of the EXTEND_MORTAL() calls I see make sense in the case where
the code might return a large number of mortals, like in pp_aassign
and pp_mapwhile, since it will avoid expensive reallocations of
PL_tmps_stack.

It's obviously much less useful in some other places​:

- pp_stat - it always returns 0 or 13 items, and 13 is well below the
minimum growth size (128)

- pp_gmtime - always extends by 1 or 9.

Do any of the other uses strike you as wasteful or incorrect?

Yes for the mortal extend by a constant under 128 yes if you are using sv_2mortal.

I think that if you will use EXTEND_MORTAL, a sv_2mortalnx should be later used. sv_2mortalnx should probably also be NN depending on what happens with an assert(sv)/make test.

I also dont like the prototype of Perl_tmps_grow. It has a void return type, the following psuedo code is the current C/machine code

---

  int ix = my_perl->Itmps_ix;
  ix += 1;
  if ( ix >= my_perl->Itmps_max )
  Perl_tmps_grow(my_perl, 1);
  ix = my_perl->Itmps_ix;
  ix += 1;
  my_perl->Itmps_ix = ix;
  my_perl->Itmps_stack[ix] = sv;

---

28094FAF and dword ptr [edi], 0 ;edi is sv, this is writing to sv_any member
28094FB2 and dword ptr [edi+sv_flags], 0
28094FB6 push 1
28094FB8 pop eax ; eax is now 0x1
28094FB9 mov [edi+sv_refcnt], eax
28094FBC mov dword ptr [edi+sv_flags], 80000h
28094FC3 mov ecx, [esi+Itmps_ix] ; esi is my_perl
28094FC6 inc ecx ;++ on tmps_ix once
28094FC7 cmp ecx, [esi+Itmps_max]
28094FCA jl short loc_28094FD5 ; jump less than
28094FCC push eax ; eax is 0x1
28094FCD push esi ; my_perl
28094FCE call _Perl_tmps_grow
28094FD3 pop ecx ;adjust C stack, ecx is garbage filled after func call, and after this op
28094FD4 pop ecx ;adjust C stack
28094FD5 loc_28094FD5​:
28094FD5 inc dword ptr [esi+Itmps_ix] ;++ on tmps_ix AGAIN!!!
28094FD8 mov eax, [esi+Itmps_ix]
28094FDB mov ecx, [esi+Itmps_stack]
28094FDE mov [ecx+eax*4], edi ; edi is SV *
28094FE1 mov eax, edi ; put SV * in return value register
28094FE3 pop edi
28094FE4 pop esi
28094FE5 retn
28094FE5 _Perl_sv_newmortal endp

---

the code should probably be like this in asm/psuedo C

  int ix = my_perl->Itmps_ix;
  ix += 1;
  my_perl->Itmps_ix = ix
  if ( ix >= my_perl->Itmps_max )
  ix = Perl_tmps_grow(my_perl, 1);
  my_perl->Itmps_stack[ix] = sv;

Another choice is so Perl_tmps_grow takes ix as its 2nd param, not the 1 or 9 or 20 or whatever count it takes right now.

This is for sv_2mortal style usage

  int ix = my_perl->Itmps_ix;
  ix += 1;
  if ( ix >= my_perl->Itmps_max )
  ix = Perl_tmps_grow(my_perl, ix);
  my_perl->Itmps_ix = ix
  my_perl->Itmps_stack[ix] = sv;

over extend would be like

  int ix = my_perl->Itmps_ix;
  ix += extend_by_n;
  if ( ix >= my_perl->Itmps_max )
  ix = Perl_tmps_grow(my_perl, ix);
  /* ix value never used/abandoned */

--
bulk88 ~ bulk88 at hotmail.com

[p5pRT]

From @bulk88

Blead on arm is worse in instruction count due to 2nd PL_temps_ix read

LDR R4, =PL_tmps_ix ; copy addr into non-vol reg R4
LDR R0, =PL_tmps_max ; copy addr into vol reg R0
LDR R3, [R4] ; load 32 bits from addr in R4(PL_tmps_ix) to R3 (ix)
LDR R1, [R0] ; load 32 bits from addr in R0(PL_tmps_max) to R1 (max)
ADD R2, R3, #1 ; R2 = ix + 1
CMP R2, R1 ; compare R2 to max
BLT $L52698 ; jump less than
MOV R0, #1 ; set 1st function call arg to 1
BL Perl_tmps_grow ; call function
LDR R3, [R4] ; load 32 bits from addr in R4(PL_tmps_ix) to R3 (ix)
$L52698
LDR R0, =PL_tmps_stack ;copy addr into vol reg R0
ADD R2, R3, #1 ;R2 = ix + 1
STR R2, [R4] ; store 32 bits from R3 (ix) to addr in R4(PL_tmps_ix)
LDR R1, [R0] ; load 32 bits from addr in R0(PL_tmps_stack) to R1 (tmps_stack)
STR R5, [R1,R2,LSL#2] ; *(tmps_stack+(ix << 2)) = sv;
LDR R0, [R5,#8] ; U32 sv_flags = *(R5+sv_flags);
ORR R1, R0, #0x80000 ;U32 new_sv_flags = sv_flags | 0x80000;
STR R1, [R5,#8] ; *(R5+sv_flags) = new_sv_flags;
MOV R0, R5 ; move SV * in R5 to return register R0
LDMFD SP, {R4,R5,SP,PC} ; restore saved registers+return to caller
; End of function Perl_sv_2mortal

--
bulk88 ~ bulk88 at hotmail.com

[p5pRT]

From @bulk88

On Thu Oct 09 00​:47​:24 2014, bulk88 wrote​:

Blead on arm is worse in instruction count due to 2nd PL_temps_ix read

cross linking #14148 - a953aca

--
bulk88 ~ bulk88 at hotmail.com

[toddr]

I don't see anything actionable here and propose closing it.