Bug 136340 – GC length unit is invalid

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Julien Brianceau]

In showObjectStatistics, GC length unit is "s" and not "ms".

Also, division by zero may occur when showObjectStatistics is called without any object stored in heap.

[Julien Brianceau]

Created attachment 237309 [details]
Correct GC length unit and prevent div by 0 in showObjectStatistics

[Mark Hahnenberg]

Comment on attachment 237309 [details]
Correct GC length unit and prevent div by 0 in showObjectStatistics

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.

[Mark Hahnenberg]

To be a little clearer with what I mean:

long wastedPropertyStorageBytes = 0;
long wastedPropertyStoragePercent = 0;
long objectsWithOutOfLineStoragePercent = 0;
if (/* we won't div-by-zero */)
    wastedPropertyStorageBytes = /* weird casts and computation here */;
    wastedPropertyStoragePercent = /* more weird casts and computation here */;
    objectsWithOutOfLineStoragePercent = /* even more weird casts and computation here */;
}

dataLogF("waste property storage: ....", wastedPropertyStorageBytes, wastedPropertyStoragePercent);
dataLogF(...);

[Julien Brianceau]

Created attachment 237310 [details]
Correct GC length unit and prevent div by 0 in showObjectStatistics (v2)

[Mark Hahnenberg]

Comment on attachment 237310 [details]
Correct GC length unit and prevent div by 0 in showObjectStatistics (v2)

Awesome, looks great. r=me

[Julien Brianceau]

Manually committed r173062: http://trac.webkit.org/changeset/173062