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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 2 06:49:03 PDT 2012


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 150415: proposed patch
https://bugs.webkit.org/attachment.cgi?id=150415&action=review

------- Additional Comments from Zoltan Horvath <zoltan at webkit.org>
(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.


More information about the webkit-reviews mailing list