[webkit-reviews] review requested: [Bug 90080] ps based memory measurement for performance-tests : [Attachment 149958] prepoposed patch

[bugzilla-daemon at webkit.org]

Zoltan Horvath <zoltan at webkit.org> has asked  for review:
Bug 90080: ps based memory measurement for performance-tests
https://bugs.webkit.org/show_bug.cgi?id=90080

Attachment 149958: prepoposed patch
https://bugs.webkit.org/attachment.cgi?id=149958&action=review

------- Additional Comments from Zoltan Horvath <zoltan at webkit.org>
(In reply to comment #3)
> It's not clear to me that there's a lot of value in hanging this off of the
driver and server_process classes, and it seems more like you're just having to
jump through some hoops to make this work at all.

You are absolutely right. I just wanted to give a shot to the problem.

> I think if we just exposed the currently running pid from the Driver object
you could implement this purely in the perftest code and we wouldn't need to
modify driver.py or server_process.py. Does that work?

The DRT process starts at webkit.py:566 in self._server_process.write(command).
>From this point we have a valid PID to the testrunner, so I start the
MemoryLogger here.

> If we did need to keep this approach, we should at least make it optional; I
don't know that we want to be running N ps processes when running layout tests
(at least not by default/always).

I think we should do this only for performance tests. Now things depend on the
--measure-memory parameter what is accessible only for run-perf-tests.

> Also, I'm not sure that you want the thread to be a daemon; is there an
advantage to that? 

If you interrupt the running of run-perf-test with ctrl+c, a non daemon thread
will be still running. If it was a daemon then it stops also.

> Lastly, you need to either figure out what the equivalent is on windows, or
at least stub it out so that we don't try to run the memory logger on windows
and keel over.

Done. :-] I mark this version to r?.

I think we should not display memory and performance results at the same time,
since running of ps may has effects on performance results. If you agree I will
upgrade the patch to support the obligated behavior. What is your opinion?

We also have to check the stdev calculation algorithm in calculate_statistics
(originally it was written by rniwa), since it gives wrong values in the case
of perftests memory e.g.
RESULT CSS: CSSPropertySetterGetter= 71920.0175246 kbytes
median= 75188 kbytes, stdev= 245500.637033 kbytes, min= 15772 kbytes, max=
76504 kbyte

Maybe I did something buggy when I put it into a function.