search

Ravenbrook / mps

The Memory Pool System
http://www.ravenbrook.com/project/mps
Other
559 stars 75 forks source link

SegSetGrey is dominating the critical path #137

Open rptb1 opened 1 year ago

rptb1 commented 1 year ago

SegSetGrey is occupying a lot of CPU time on the critical path in sampling profiles.

keybase://chat/ravenbrook#mps/2232 by @rptb1 of 2022-12-14 16:59 UTC says:

Hmm... gprof says

  %   cumulative   self              self     total           
  4.96      3.09     0.24 15165864     0.00     0.00  SegSetGrey

which is a bit surprising. Changing the AVERs in SegSetGrey to CRITICAL. Before:

rb@plover:~/git/mps/code$ time lii6gc/hot/gcbench -x 6415986 amc
amc: 7.37883

real  0m7,398s
user  0m7,196s
sys   0m0,201s

after:

rb@plover:~/git/mps/code$ time lii6gc/hot/gcbench -x 6415986 amc
amc: 5.70199

real  0m5,729s
user  0m5,353s
sys   0m0,376s

Do I understand this properly? Nope!

rptb1 commented 1 year ago

This performance regression was introduced gradually by several changes. SegSetGrey was recognized as being on the critical path in the past but extra assertions were introduced with segment classes.

Compare https://github.com/Ravenbrook/mps/blob/081d79ee2299509615b67cdbe8f0bfdfbd93feab/code/seg.c#L1578-L1585 with https://github.com/Ravenbrook/mps/blob/081d79ee2299509615b67cdbe8f0bfdfbd93feab/code/seg.c#L237-L254

We could track down where this happened if we really wanted.

rptb1 commented 1 year ago

Some suggestions for prevention:

  1. Extra review rules for the critical path. I think this has been suggested by @thejayps in another context.
  2. Better profiling support. We can try to make profiling much easier for developers so that they can at least eyeball it before committing changes or submitting pull requests. gp.gmk has gone rusty for a start (see keybase://chat/ravenbrook#mps/2234 )
  3. Automated performance regression tests.
rptb1 commented 1 year ago

2. Better profiling support.

See also https://github.com/Ravenbrook/mps/pull/138#issuecomment-1422704228

rptb1 commented 1 year ago

Consider shorting out the segment class dispatch (calling gcSegSetGrey directly) here (and similar) https://github.com/Ravenbrook/mps/blob/9bc668f7ef5284c624504d9c17e015c4167011ec/code/poolamc.c#L1651 We know that the destination toSeg is a GC segment.