Bug 172186 – [iOS] The Garbage Collector shouldn't rely on the bmalloc scavenger for up to date memory footprint info

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

Change set r216763: <http://trac.webkit.org/changeset/216763> introduced code to read memory footprint info from the kernel and respond when the process's memory allocation is getting close to the expected available memory.  In that change set, the GC relies on scavenger updating the footprint value in timely manner.  That is not always possibly, primarily due to allocation patterns.  The GC code should also update the footprint value in a timely manner.

[Radar WebKit Bug Importer]

<rdar://problem/32231531>

[Michael Saboff]

Created attachment 310304 [details]
Patch

[Build Bot]

Attachment 310304 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/Heap.h:80:  The parameter name "memoryDataAge" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/bmalloc/bmalloc/Heap.h:81:  The parameter name "memoryDataAge" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 310304 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310304&action=review

> Source/JavaScriptCore/heap/Heap.cpp:519
> -    if (bmalloc::api::percentAvailableMemoryInUse() > Options::criticalGCMemoryThreshold())
> +    static unsigned memCalls = 0;
> +
> +    bmalloc::MemoryDataAge memoryDataAge = bmalloc::MemoryDataAge::UseCached;
> +
> +    if (++memCalls >= 100) {
> +        memoryDataAge = bmalloc::MemoryDataAge::GetCurrent;
> +        memCalls = 0;
> +    }
> +
> +    if (bmalloc::api::percentAvailableMemoryInUse(memoryDataAge) > Options::criticalGCMemoryThreshold())
>          return true;

This is a nice heuristic!

[Geoffrey Garen]

r=me

> Source/JavaScriptCore/heap/Heap.cpp:509
> +    static unsigned memCalls = 0;

Please move this value to be a heap data member so that it's thread safe.

> Source/JavaScriptCore/heap/Heap.cpp:519
> +    if (bmalloc::api::percentAvailableMemoryInUse(memoryDataAge) > Options::criticalGCMemoryThreshold())
>          return true;

What's the point of consulting this value in the cached case? Won't we make the exact same policy decision we made the last time around?

I think it would be clearer just to return early with false in the < 100 case. That would also remove the need for any caching API: The public bmalloc API would always return an up-to-date value.

[Michael Saboff]

(In reply to Geoffrey Garen from comment #5)
> r=me
> 
> > Source/JavaScriptCore/heap/Heap.cpp:509
> > +    static unsigned memCalls = 0;
> 
> Please move this value to be a heap data member so that it's thread safe.

Will do.

> > Source/JavaScriptCore/heap/Heap.cpp:519
> > +    if (bmalloc::api::percentAvailableMemoryInUse(memoryDataAge) > Options::criticalGCMemoryThreshold())
> >          return true;
> 
> What's the point of consulting this value in the cached case? Won't we make
> the exact same policy decision we made the last time around?
> 
> I think it would be clearer just to return early with false in the < 100
> case. That would also remove the need for any caching API: The public
> bmalloc API would always return an up-to-date value.

Because the scavenger could update the value between direct calls.  We want the GC to adjust as soon as it notices we are critical.

[Geoffrey Garen]

> Because the scavenger could update the value between direct calls.  We want
> the GC to adjust as soon as it notices we are critical.

The scavenger's job is to run when memory use goes down. If we're relying on the scavenger to notice when memory use goes up, we have a bad design.

Does checking the scavenger's cached value actually decrease the jetsam rate?

[Michael Saboff]

(In reply to Geoffrey Garen from comment #7)
> > Because the scavenger could update the value between direct calls.  We want
> > the GC to adjust as soon as it notices we are critical.
> 
> The scavenger's job is to run when memory use goes down. If we're relying on
> the scavenger to notice when memory use goes up, we have a bad design.

Logging shows that sometimes the scavenger runs and memory use doesn't go down.  Consider that the scavenger was scheduled due to blocks being put on the free list.  Prior to the scavenger actually running, those blocks are reused.  The scavenger may end up doing nothing and in fact memory use could grow due to more blocks being allocated.  In those cases, the scavenger does update the memory footprint and it could be advantageous to the GC.

> Does checking the scavenger's cached value actually decrease the jetsam rate?

In my testing this patch doesn't affect the jetsam rate one way or the other.  I am not seeing jetsams before or after this change when running JetStream, the benchmark that appears to have the highest memory use.

[Michael Saboff]

Created attachment 310416 [details]
Updated Patch

This patch implements what ggaren is asking for, specifically that the GC doesn't rely on cached values from the scavenger.  The GC implements its own cache of overCriticalMemoryThreshold and gets that value fresh at the end of collection for the next cycle.  The cache is still updated 1 out of every 100 calls to overCriticalMemoryThreshold().

[Build Bot]

Attachment 310416 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/Heap.h:540:  The parameter name "memoryThresholdCallType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 310416 [details]
Updated Patch

r=me

[Build Bot]

Comment on attachment 310416 [details]
Updated Patch

Attachment 310416 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3764506

New failing tests:
http/tests/appcache/404-resource-with-slow-main-resource.php

[Build Bot]

Created attachment 310440 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Michael Saboff]

Committed r217000: <http://trac.webkit.org/changeset/217000>