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.
<rdar://problem/32231531>
Created attachment 310304 [details] Patch
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.
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!
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.
(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.
> 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?
(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.
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().
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.
Comment on attachment 310416 [details] Updated Patch r=me
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
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
Committed r217000: <http://trac.webkit.org/changeset/217000>