708082 - Linux: Improve AVMPI_getPrivateResidentPageCount

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

Description

[Bug Lawnmower (tlt)]

Attachment: linux_AVMPI_getPrivateResidentPageCount.LT.patch

User Agent: Mozilla/5.0 (compatible; MSIE 9.0; Windows NT 6.1; Win64; x64; Trident/5.0; .NET CLR 2.0.50727; SLCC2; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; Tablet PC 2.0)

Steps to reproduce:

The current AVMPI_getPrivateResidentPageCount for Linux takes a scenic route to perform its duty - reading (in several small blocks) and parsing (in a state machine) the contents of /proc/PID/smaps. A much shorter and simpler route is available in the form of /proc/self/statm.
This patch gives a bit of performance improvement (its called frequently by MMgc), but perhaps most important is that it vastly reduces the clutter in the logs produced by strace(1) and similar tools.

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

(someone with appropriate priveleges should give this a whirl through the buildbot.  if it hasn't happened by tomorrow morning CET, I'll give it a shot.)

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 579445 [details] [diff] [review]
linux_AVMPI_getPrivateResidentPageCount.LT.patch

Patch looks plausible.  I'm putting it through a buildbot run now.

Nit: Prevailing style is to not insert spaces in between variable and ++ operator.  I assume we'll fix that before pushing to TR repo.

My main question is whether we'd get into trouble on systems that do not offer /proc/self/statm with contents that conform to the format expected by the parsing here.  Both the old code and the new code made assumptions about the input format, so its really more question of whether (1.) one format is more prevalent than the other, and (2.) what ill effects could arise when we bail out?

Comment 3

[Bug Lawnmower (tlt)]

/proc is the actual OS API to get this kind of information. It just gets this appearance of being an adventure into uncharted realms where there be dragons because of the 'everything is a file' philosophy ('get a byte, getabytebytebyte'), and my obsessively paranoid parsing ('get a shrink, getashrinkshrinkshrink'). 

The difference between them is simply that 'statm' gives a brief process-wide summary of memory use (as a single line of unlabeled values), while 'smaps' lists detailed info for each mapping (on individual labelled rows, making it much easier to find). Note now both formats by design allow adding new fields without breaking old code.
And last but not least- 'self' simply points to /proc/<current PID>.

If things broke in any significant and/or hilarious way if the info is unavailable (rare, but possible - /proc not mounted, process chroot'ed or otherwise restricted, etc) it would have been noticed by now...

(Also, I've actually tested.)

Comment 4

[Bug Lawnmower (tlt)]

(melody: Minuit - Wayho)

Who will, who will, review this patch?
Who will, who will, review and push?

This little patch is in need of a pushing
A single command, few keys need pressing
It's tested in production - I won't be blushing
But review is compulsory, 
so - who will, who will?

Who will, who will, review this patch?
Who will, who will, review and push?

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

(i'll try to attack this today; I need to resolve Bug 559696 (mainly Bug 656008) first just to get things building in my linux VMware image.)

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 579445 [details] [diff] [review]
linux_AVMPI_getPrivateResidentPageCount.LT.patch

Indeed, quite a speed improvement on test/performance/mmgc/*.as when one has --memstats turned on.  (As in, total time for five iterations in runtests.py with --memory option goes from two minutes down to 1m26sec, though those numbers should be taken with a grain of salt for many reasons.)

Speed improvement is not nearly as dramatic when one is not using --memstats, which is expected since there are presumably far fewer invocations of getPrivateResidentPageCount.

The reported results generally look about the same, which is the thing I was most concerned about.

I'll put it through another buildbot whirl (I've forgotten the results of the one back in comment 2).  If that goes well, I'll push it.

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch L v2: direct parsing of /proc/self/statm

patch L v2: cleaned up indentation/whitespace of prior version; otherwise identical to prior version (just stashing for safe keeping).

Comment 8

[Tamarin Bot]

changeset: 7190:fc1165800d25
user:      Bug Lawnmower (tlt) <[e-mail]>
summary:   Bug 708082: patch L v2: direct parsing of /proc/self/statm (r=fklockii)

http://hg.mozilla.org/tamarin-redux/rev/fc1165800d25

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

I might need to back this out, at least one benchmark is reporting memory regression with the change in the place.  The difference is definitely isolatable to the changeset from comment 8.  I have not yet managed to isolate whether the supposed regression is a artifact of the change in reporting mechanism, or if the actual memory usage has changed because of chaotic policy changes in response to the changes in the reporting mechanism and the running times of the reporting mechanism.  (The latter seems unlikely but I'm keeping an open mind.)

Comment 10

[Bug Lawnmower (tlt)]

(melody: Front 242 - Quite Unusual)
the tests went on and mem use started sort of growing
a bloating fright tore across the devs
Felix swept the patch out
and left it completely flattened out
and several questions appeared like gigantic flowers 

the way the stats broke was quite unusual

i should have counted only private pages
but this was no concern of mine
so the result was growing 

my eyes roamed over procfs source and the tests
and i felt really dumb, no wonder it's oversized

the way the morning broke was quite unusual
new patch is on its way
(it's attached)

Comment 11

[Bug Lawnmower (tlt)]

Attachment: version that doesnt count shared pages

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 643429 [details] [diff] [review]
version that doesnt count shared pages

Tony: this is a patch that is only relevant to Linux targets, e.g. Android, AIR on Linux (does that exist?), and some things like I think Digital Home.  I kept putting off reviewing it because I wanted to take the proper time to check whether it changed our results significantly, something I failed to do the last time around on this ticket, which led to my retracting this patch.

Since it is only relevant to Linux, If you are going to evaluate it, you should make sure to either evaluate it in a Linux VM image, or hand it off to someone who has a Linux machine available for testing on and enjoys using Linux.  I think Mathew Zaleski (I put him down for a feedback request) could be an option for that.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.