Bug 90080

Created attachment 149759 [details] first solution for discussion This patch has some limitations but you can try ps based memory measurement either with perftest or with pageloadtests as well. I start a MemoryLogger thread when we start the DRT then I poll ps until the tests are running. What is your opinion about this approach?

Comment on attachment 149759 [details] first solution for discussion View in context: https://bugs.webkit.org/attachment.cgi?id=149759&action=review > Tools/Scripts/webkitpy/performance_tests/perftest.py:152 > + def calculate_results(self, input_list, unit): I would call this calculate_statistics.

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. 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? 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). Also, I'm not sure that you want the thread to be a daemon; is there an advantage to that? 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. Otherwise the code looks reasonable :).

Created attachment 149958 [details] prepoposed patch (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.

Attachment 149958 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1 Tools/Scripts/webkitpy/performance_tests/perftest.py:308: missing whitespace after ',' [pep8/E231] [5] Tools/Scripts/webkitpy/layout_tests/port/webkit.py:569: multiple spaces after operator [pep8/E222] [5] Tools/Scripts/webkitpy/performance_tests/memorylogger.py:36: expected 2 blank lines, found 1 [pep8/E302] [5] Tools/Scripts/webkitpy/performance_tests/memorylogger.py:64: blank line at end of file [pep8/W391] [5] Total errors found: 4 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 149962 [details] prepoposed patch + style fixes

Comment on attachment 149962 [details] prepoposed patch + style fixes View in context: https://bugs.webkit.org/attachment.cgi?id=149962&action=review > Tools/Scripts/webkitpy/layout_tests/port/driver.py:37 > + def __init__(self, test_name, timeout, image_hash, should_run_pixel_test, should_measure_memory=None, args=None): Nit: please use should_measure_memory=False for this.

(In reply to comment #4) > > 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. > True. Normally I like to catch ctrl-c directly in my code and clean up. > 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? > I defer this to rniwa, but it seems sensible to me. the code looks fine to me but I'll let rniwa give the r+ if he's happy also.

Comment on attachment 149962 [details] prepoposed patch + style fixes Can I see some sample outputs?

Created attachment 150091 [details] sample output (performance tests + pageload tests)*memory results Thanks guys for the comments! Currently, the output is the same without the --memory-measure parameter. I attached an output file of the --memory-measure case for the performance tests and the pageload tests run. We should fix the stdev calculation, but it should be a separate bug.

View in context: https://bugs.webkit.org/attachment.cgi?id=149962&action=review Just some observations: > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:569 > + memlogger = MemoryLogger(self._server_process._pid) use 'pid()' instead of '_pid'. > Tools/Scripts/webkitpy/performance_tests/memorylogger.py:34 > +import time > +import threading > +import subprocess > +import sys > +import re Should this be in alphabetic order? > Tools/Scripts/webkitpy/performance_tests/memorylogger.py:45 > + while True and not self._stop: I think you can skip the 'True'. > Tools/Scripts/webkitpy/performance_tests/perftest.py:226 > + if len(memory_values): No need for the len().

Created attachment 150415 [details] proposed patch (In reply to comment #7) > (From update of attachment 149962 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149962&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/driver.py:37 > > + def __init__(self, test_name, timeout, image_hash, should_run_pixel_test, should_measure_memory=None, args=None): > > Nit: please use should_measure_memory=False for this. Thanks Dirk, fixed! (In reply to comment #11) > View in context: https://bugs.webkit.org/attachment.cgi?id=149962&action=review > > Just some observations: > > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:569 > > + memlogger = MemoryLogger(self._server_process._pid) > > use 'pid()' instead of '_pid'. Fixed. > > Tools/Scripts/webkitpy/performance_tests/memorylogger.py:34 > > +import time > > +import threading > > +import subprocess > > +import sys > > +import re > > Should this be in alphabetic order? Fixed. > > Tools/Scripts/webkitpy/performance_tests/memorylogger.py:45 > > + while True and not self._stop: > > I think you can skip the 'True'. > > > Tools/Scripts/webkitpy/performance_tests/perftest.py:226 > > + if len(memory_values): > > No need for the len(). Fixed. Thanks for the comments Mr.Python ;] Ryosuke, it's your turn now.

Comment on attachment 150415 [details] proposed patch Attachment 150415 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13134096 New failing tests: fast/loader/loadInProgress.html

Created attachment 150418 [details] Archive of layout-test-results from gce-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

I was thinking about how can we make the results more exact. Now, the useful numbers from the memory statistics are the max and the average, but the average value is not correct enough, because while we monitor the memory usage the memory usage ideally always starts from 0 kbytes and ends at the maxRSS. In the case of pageloadtests we load the about:blank page before every test. What if we do the same for the simple perftests? Then we can measure the actual memory consumption for the about:blank page (it will be always lower than any other page loading) and in the case of the real test during the statistics calculation we'd count only with the higher memory consumption values than the about:blank's consumption. With this approach we can decrease the standard deviation and the average value will be much correct.

if we do that, then don't we end up favoring changes that increase the memory consumption on about:blank?

(In reply to comment #16) > if we do that, then don't we end up favoring changes that increase the memory consumption on about:blank? It can happen, but I think it's not significant, loading of about:blank consumes always less memory than loading a normal page. What is your opinion about the uploaded solution?

(In reply to comment #17) > It can happen, but I think it's not significant, loading of about:blank consumes always less memory than loading a normal page. Well maybe. But that means any regressions that increase the memory usage in about:blank will show up as an improvement. That doesn't seem like a good idea. > What is your opinion about the uploaded solution? It seems reasonable but I'm not sure how we're going to use it given you mention that "ps" will affect the performance of the machine and therefore we need to run a separate instance. I'm inclined to say that we should just start with FastMalloc statistics since that'll have no overhead like this approach.

(In reply to comment #18) > (In reply to comment #17) > > It can happen, but I think it's not significant, loading of about:blank consumes always less memory than loading a normal page. > > Well maybe. But that means any regressions that increase the memory usage in about:blank will show up as an improvement. That doesn't seem like a good idea. Good point. > > What is your opinion about the uploaded solution? > > It seems reasonable but I'm not sure how we're going to use it given you mention that "ps" will affect the performance of the machine and therefore we need to run a separate instance. I'm inclined to say that we should just start with FastMalloc statistics since that'll have no overhead like this approach. Okay, let's leave this approach here. I'm going to open a new bug for FastMalloc stats and examine the possibilities how to measure the memory with it.

(In reply to comment #19) > > It seems reasonable but I'm not sure how we're going to use it given you mention that "ps" will affect the performance of the machine and therefore we need to run a separate instance. I'm inclined to say that we should just start with FastMalloc statistics since that'll have no overhead like this approach. > > Okay, let's leave this approach here. I'm going to open a new bug for FastMalloc stats and examine the possibilities how to measure the memory with it. With FastMallocStatistics we can do the same with ps, so we need to monitor the memory usage periodically, so I need to dedicate a thread to query the statistics. Do you think that a dedicated memory-monitor thread has less affect on the performance than a dedicated process? *FastMallocStatistics* Cons: - it doesn't include the memory usage of the JS heap since it has its own dedicated memory. - it includes only heap allocated objects - only work on the platforms that uses TCmalloc - we need to modify DRT/WTR to get the results properly Pros: - system independent I think if we don't want to affect on the performance results we need to run a separate instance of the performance tests. I know that is too much time, since the performance tests run more than 45 mins... We either need to live with the minimal affect or allow a separate run.

(In reply to comment #20) > With FastMallocStatistics we can do the same with ps, so we need to monitor the memory usage periodically, so I need to dedicate a thread to query the statistics. Do you think that a dedicated memory-monitor thread has less affect on the performance than a dedicated process? I had discussed this with anttik before, and we concluded that we can measure the result at the very end. While it might be interesting to measure things periodically, I'd suspect that the values will vary so much between each run that it won't be usable. On the other hand, the final value we obtain when the page is fully loaded will give us a good sense of how much RAM is used when the page is loaded, which is pretty useful thing to know. > - it doesn't include the memory usage of the JS heap since it has its own dedicated memory. Doesn't JSC also provide some API/stat for its memory usage?

Comment on attachment 150415 [details] proposed patch Remove r?, since we landed FastMalloc + JS Heap based approach in http://trac.webkit.org/changeset/125178.

Closing as wontfix.