[Valgrind-developers] Another architectural thing ...

[Julian S. <js...@ac...>]

While you're on the software-structure warpath, here's something
I noticed a while back, around xmas, when JeremyF and me were trying
to improve performance.

A big bottleneck (we surmise, but haven't accurately measured)
for skins which deal with A bits, is handling %ESP changes.  

The core/skin split has made this more expensive, I think, by
causing longer chains of calls into the relevant skin's memory-
permissions adjustor functions.  That isn't the problem, tho;
the following nonsensicality was there prior to that.

Consider a source push instr:

        0x40184623:  pushl %esi

          10: GETL      %ESI, t8
          11: GETL      %ESP, t12
          12: MOVL      t12, t10
          13: SUBL      $0x4, t10
          14: PUTL      t10, %ESP
          15: STL       t8, (t10)
          16: INCEIPo   $1

Eventually there is, a result of this, a call to VG_(handle_esp_assignment),
assuming the skin has asked for shadow memory (aiui).  This passes the
old and new %ESP values, and in VG_(handle_esp_assignment) these are 
subtracted to discover that the stack delta is -4.  Then we do

        VG_TRACK(new_mem_stack_aligned, new_esp, -delta); 
                                          (vg_memory.c:320ish)

which necessarily involve a loop, and probably a test on the sign of
delta, in the relevant skin's permissions-mangling function.

This is all rather inefficient considering that we knew from the start
that delta would be -4, and it would have been better to plant, in the
generated code, a call directly to a skin function specialised to a delta
of -4.  Most deltas are small (+4, -4, +16, -16) and so a small bunch of
specialised functions + a general fallback case would probably constitute
a significant improvement.

The improvements would be because (1) the specialised functions have no
need to test the sign of delta nor have a loop to handle all values of
it, and (2) because we could save having to call VG_(handle_esp_assignment)
and then call onwards to the skin function; we could just call the skin
function directly.

You're the Architect(tm); I don't know if the idea makes sense or would
cause other difficulties, but I did notice this a while back.

J