[webkit-reviews] review requested: [Bug 90858] JSHeap and FastMallocStatistics based memory measurement for performance-tests : [Attachment 157145] proposed patch

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Zoltan Horvath <zoltan at webkit.org>
(In reply to comment #18)
> (From update of attachment 156983 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=156983&action=review
> 
> > PerformanceTests/resources/runner.js:126
> > +	     this.logStatistics(this._results, this.unit, "Performance:");
> 
> It's probably to call this "Time" because fps, runs/s, ms, are all about time
and frequency.

Fixed.

> > PerformanceTests/resources/runner.js:128
> > +	     this.logStatistics(this._jsHeapResults, "bytes", "JS heap
usage:");
> > +	     this.logStatistics(this._fastMallocHeapResults, "bytes",
"FastMalloc heap usage:");
> 
> I would call these "JS Heap" and "FastMalloc" since the fact that these
numbers are "usage" is quite self-evident.

Fixed.

> > PerformanceTests/resources/runner.js:175
> > +// This function is to be used in fps based performance tests
> 
> I don't think this comment is accurate; it could be used on other tests as
well.

Removed.

> > Tools/Scripts/webkitpy/performance_tests/perftest.py:128
> > +	     actual_result_class = ""
> 
> Prefix "actual" is somewhat confusing because there isn't "expected" result
class or anything.

Fixed.
 
> > Tools/Scripts/webkitpy/performance_tests/perftest.py:136
> > +		 tmp_result_class = result_class_regex.match(line)
> > +		 if tmp_result_class:
> 
> I would call this result_class_match. Also, if you're sticking with tmp_,
then please spell out temp_.

Done.

> Can we use a dictionary of dictionaries here so that we can just do
> results[result_class]['unit'] = unit
> results[result_class][key] = key
> ?

You are absolutely right. Fixed.
 
> > Tools/Scripts/webkitpy/performance_tests/perftest.py:168
> > +	     self.output_statistics(test_name + ": FastMallocHeap",
fastmalloc_memory_results, description_string)
> 
> I don't think we need to have Heap postfix for FastMalloc because we don't
use fast malloc for non-heap memory allocation.

Fixed.

> > Tools/Scripts/webkitpy/performance_tests/perftest.py:176
> > +	     _log.info(', '.join(['%s= %s %s' % (key, results[key], unit) for
key in self._statistics_keys[1:5]]))
> 
> Why do we need to limit at :5?

Because I put unit into statistics_keys and I don't want to print it twice.

Thanks for the review, new patch is uploaded.