[webkit-reviews] review denied: [Bug 136340] GC length unit is invalid : [Attachment 237309] Correct GC length unit and prevent div by 0 in showObjectStatistics

[bugzilla-daemon at webkit.org]

Mark Hahnenberg <mhahnenb at gmail.com> has denied Julien Brianceau
<jbriance at cisco.com>'s request for review:
Bug 136340: GC length unit is invalid
https://bugs.webkit.org/show_bug.cgi?id=136340

Attachment 237309: Correct GC length unit and prevent div by 0 in
showObjectStatistics
https://bugs.webkit.org/attachment.cgi?id=237309&action=review

------- Additional Comments from Mark Hahnenberg <mhahnenb at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=237309&action=review


This looks like a good change, but I think we can make it even better and be
good citizens in the process :-)

> Source/JavaScriptCore/heap/HeapStatistics.cpp:256
> +    if ((storageStatistics.storageCapacity() > 0) &&
(storageStatistics.objectCount() > 0)) {
> +	   dataLogF("wasted .property storage: %ldkB (%ld%%)\n",
> +	       static_cast<long>(
> +		   (storageStatistics.storageCapacity() -
storageStatistics.storageSize()) / KB),
> +	       static_cast<long>(
> +		   (storageStatistics.storageCapacity() -
storageStatistics.storageSize()) * 100
> +		       / storageStatistics.storageCapacity()));
> +	   dataLogF("objects with out-of-line .property storage: %ld
(%ld%%)\n",
> +	       static_cast<long>(
> +		   storageStatistics.objectWithOutOfLineStorageCount()),
> +	       static_cast<long>(
> +		   storageStatistics.objectWithOutOfLineStorageCount() * 100
> +		       / storageStatistics.objectCount()));
> +    }

This is very weirdly formatted code. Maybe we could clean it up a little since
we're already here.

I'd suggest computing the wasted property storage and storing it in a local
variable, then printing it out in a normal looking dataLogF below. You can use
control flow to avoid the div-by-zero issue when computing that value. Ditto
for objects w/ out-of-line property storage.