Index: source/gui/GUIManager.cpp =================================================================== --- source/gui/GUIManager.cpp +++ source/gui/GUIManager.cpp @@ -379,7 +379,7 @@ // We share the script context with everything else that runs in the same thread. // This call makes sure we trigger GC regularly even if the simulation is not running. - m_ScriptInterface->GetContext()->MaybeIncrementalGC(1.0f); + m_ScriptInterface->GetContext()->MaybeIncrementalGC(); // Save an immutable copy so iterators aren't invalidated by tick handlers PageStackType pageStack = m_PageStack; Index: source/network/NetServer.cpp =================================================================== --- source/network/NetServer.cpp +++ source/network/NetServer.cpp @@ -445,7 +445,7 @@ // (Do as little work as possible while the mutex is held open, // to avoid performance problems and deadlocks.) - m_ScriptInterface->GetContext()->MaybeIncrementalGC(0.5f); + m_ScriptInterface->GetContext()->MaybeIncrementalGC(); ScriptRequest rq(m_ScriptInterface); Index: source/scriptinterface/ScriptContext.h =================================================================== --- source/scriptinterface/ScriptContext.h +++ source/scriptinterface/ScriptContext.h @@ -56,16 +56,22 @@ /** - * MaybeIncrementalGC tries to determine whether a context-wide garbage collection would free up enough memory to - * be worth the amount of time it would take. It does this with our own logic and NOT some predefined JSAPI logic because - * such functionality currently isn't available out of the box. - * It does incremental GC which means it will collect one slice each time it's called until the garbage collection is done. - * This can and should be called quite regularly. The delay parameter allows you to specify a minimum time since the last GC - * in seconds (the delay should be a fraction of a second in most cases though). - * It will only start a new incremental GC or another GC slice if this time is exceeded. The user of this function is - * responsible for ensuring that GC can run with a small enough delay to get done with the work. + * MaybeIncrementalGC checks if running a GC is worth the time that will take. + * The logic is custom as Spidermonkey tends to assume 'idle time' will exist, + * which is a thing in websites but not really in 0 A.D. + * This can have a few behaviours: + * - doing nothing + * - starting a new incremental GC + * - running a GC slice + * - finishing the incremental GC + * For details, check the SM doc in e.g. GC.cpp and GCapi.cpp + */ + void MaybeIncrementalGC(); + + /** + * Does a non-incremental, shrinking GC. + * A shrinking GC dumps JIT code and tries to defragment memory. */ - void MaybeIncrementalGC(double delay); void ShrinkingGC(); /** Index: source/scriptinterface/ScriptContext.cpp =================================================================== --- source/scriptinterface/ScriptContext.cpp +++ source/scriptinterface/ScriptContext.cpp @@ -156,105 +156,71 @@ } #define GC_DEBUG_PRINT 0 -void ScriptContext::MaybeIncrementalGC(double delay) +void ScriptContext::MaybeIncrementalGC() { PROFILE2("MaybeIncrementalGC"); - if (JS::IsIncrementalGCEnabled(m_cx)) - { - // The idea is to get the heap size after a completed GC and trigger the next GC when the heap size has - // reached m_LastGCBytes + X. - // In practice it doesn't quite work like that. When the incremental marking is completed, the sweeping kicks in. - // The sweeping actually frees memory and it does this in a background thread (if JS_USE_HELPER_THREADS is set). - // While the sweeping is happening we already run scripts again and produce new garbage. - - const js::SliceBudget GCSliceTimeBudget = js::SliceBudget(js::TimeBudget(30)); // Milliseconds an incremental slice is allowed to run + if (!JS::IsIncrementalGCEnabled(m_cx)) + return; - // Have a minimum time in seconds to wait between GC slices and before starting a new GC to distribute the GC - // load and to hopefully make it unnoticeable for the player. This value should be high enough to distribute - // the load well enough and low enough to make sure we don't run out of memory before we can start with the - // sweeping. - if (timer_Time() - m_LastGCCheck < delay) - return; + // The idea is to get the heap size after a completed GC and trigger the next GC + // when the heap size has reached m_LastGCBytes + X. + // Spidermonkey allocates memory arenas of 4KB for JS heap data. + // At the end of a GC, any such arena that became empty is freed. + // On shrinking GCs, spidermonkey further defragments the arenas, which effectively frees more memory but costs time. + // In practice, shrinking GCs also dump JITted code and the defragmentation is not worth it for 0 A.D. + // The regular GCs also free quite a bit of memory anyways, and non-full arenas get used for new objects. - m_LastGCCheck = timer_Time(); - - int gcBytes = JS_GetGCParameter(m_cx, JSGC_BYTES); + int gcBytes = JS_GetGCParameter(m_cx, JSGC_BYTES); #if GC_DEBUG_PRINT - std::cout << "gcBytes: " << gcBytes / 1024 << " KB" << std::endl; + printf("gcBytes: %i KB, last of %i KB\n", gcBytes / 1024, m_LastGCBytes / 1024); #endif - if (m_LastGCBytes > gcBytes || m_LastGCBytes == 0) - { + // The memory freeing happens mostly in the background, so we can't rely on the value on the last incremental slice. + // To fix that, just remember a 'minimum' value. + if (m_LastGCBytes > gcBytes || m_LastGCBytes == 0) + { #if GC_DEBUG_PRINT - printf("Setting m_LastGCBytes: %d KB \n", gcBytes / 1024); + printf("Setting m_LastGCBytes: %d KB \n", gcBytes / 1024); #endif - m_LastGCBytes = gcBytes; - } + m_LastGCBytes = gcBytes; + } - // Run an additional incremental GC slice if the currently running incremental GC isn't over yet - // ... or - // start a new incremental GC if the JS heap size has grown enough for a GC to make sense - if (JS::IsIncrementalGCInProgress(m_cx) || (gcBytes - m_LastGCBytes > m_HeapGrowthBytesGCTrigger)) - { + // Run an additional incremental GC slice if the currently running incremental GC isn't over yet + // ... or + // start a new incremental GC if the JS heap size has grown enough for a GC to make sense + if (JS::IsIncrementalGCInProgress(m_cx) || (gcBytes - m_LastGCBytes > m_HeapGrowthBytesGCTrigger)) + { #if GC_DEBUG_PRINT - if (JS::IsIncrementalGCInProgress(m_cx)) - printf("An incremental GC cycle is in progress. \n"); - else - printf("GC needed because JSGC_BYTES - m_LastGCBytes > m_HeapGrowthBytesGCTrigger \n" - " JSGC_BYTES: %d KB \n m_LastGCBytes: %d KB \n m_HeapGrowthBytesGCTrigger: %d KB \n", - gcBytes / 1024, - m_LastGCBytes / 1024, - m_HeapGrowthBytesGCTrigger / 1024); + if (JS::IsIncrementalGCInProgress(m_cx)) + printf("An incremental GC cycle is in progress. \n"); + else + printf("GC needed because JSGC_BYTES - m_LastGCBytes > m_HeapGrowthBytesGCTrigger \n" + " JSGC_BYTES: %d KB \n m_LastGCBytes: %d KB \n m_HeapGrowthBytesGCTrigger: %d KB \n", + gcBytes / 1024, + m_LastGCBytes / 1024, + m_HeapGrowthBytesGCTrigger / 1024); #endif - // A hack to make sure we never exceed the context size because we can't collect the memory - // fast enough. - if (gcBytes > m_ContextSize / 2) - { - if (JS::IsIncrementalGCInProgress(m_cx)) - { -#if GC_DEBUG_PRINT - printf("Finishing incremental GC because gcBytes > m_ContextSize / 2. \n"); -#endif - PrepareZonesForIncrementalGC(); - JS::FinishIncrementalGC(m_cx, JS::GCReason::API); - } - else - { - if (gcBytes > m_ContextSize * 0.75) - { - ShrinkingGC(); #if GC_DEBUG_PRINT - printf("Running shrinking GC because gcBytes > m_ContextSize * 0.75. \n"); + if (!JS::IsIncrementalGCInProgress(m_cx)) + printf("Starting incremental GC \n"); + else + printf("Running incremental GC slice \n"); #endif - } - else - { -#if GC_DEBUG_PRINT - printf("Running full GC because gcBytes > m_ContextSize / 2. \n"); -#endif - JS_GC(m_cx); - } - } - } - else - { -#if GC_DEBUG_PRINT - if (!JS::IsIncrementalGCInProgress(m_cx)) - printf("Starting incremental GC \n"); - else - printf("Running incremental GC slice \n"); -#endif - PrepareZonesForIncrementalGC(); - if (!JS::IsIncrementalGCInProgress(m_cx)) - JS::StartIncrementalGC(m_cx, JS::GCOptions::Normal, JS::GCReason::API, GCSliceTimeBudget); - else - JS::IncrementalGCSlice(m_cx, JS::GCReason::API, GCSliceTimeBudget); - } - m_LastGCBytes = gcBytes; - } + + // There is a tradeoff between this time and the number of frames we must run GCs on, but overall we should prioritize smooth framerates. + const js::SliceBudget GCSliceTimeBudget = js::SliceBudget(js::TimeBudget(3)); // Milliseconds an incremental slice is allowed to run + + PrepareZonesForIncrementalGC(); + if (!JS::IsIncrementalGCInProgress(m_cx)) + JS::StartIncrementalGC(m_cx, JS::GCOptions::Normal, JS::GCReason::API, GCSliceTimeBudget); + else + JS::IncrementalGCSlice(m_cx, JS::GCReason::API, GCSliceTimeBudget); + + // Reset this here so that the minimum gets cleared. + m_LastGCBytes = gcBytes; } } Index: source/simulation2/Simulation2.cpp =================================================================== --- source/simulation2/Simulation2.cpp +++ source/simulation2/Simulation2.cpp @@ -1,4 +1,4 @@ -/* Copyright (C) 2022 Wildfire Games. +/* Copyright (C) 2023 Wildfire Games. * This file is part of 0 A.D. * * 0 A.D. is free software: you can redistribute it and/or modify @@ -493,19 +493,9 @@ } // Run the GC occasionally - // No delay because a lot of garbage accumulates in one turn and in non-visual replays there are - // much more turns in the same time than in normal games. - // Every 500 turns we run a shrinking GC, which decommits unused memory and frees all JIT code. - // Based on testing, this seems to be a good compromise between memory usage and performance. - // Also check the comment about gcPreserveCode in the ScriptInterface code and this forum topic: - // http://www.wildfiregames.com/forum/index.php?showtopic=18466&p=300323 - // // (TODO: we ought to schedule this for a frame where we're not // running the sim update, to spread the load) - /*if (m_TurnNumber % 500 == 0) - scriptInterface.GetContext()->ShrinkingGC(); - else*/ - scriptInterface.GetContext()->MaybeIncrementalGC(0.0f); + scriptInterface.GetContext()->MaybeIncrementalGC(); if (m_EnableOOSLog) DumpState();