Differential D1073

Delete obsolete unused JS Garbage Collection function variations
ClosedPublic
Actions

Authored by elexis on Nov 26 2017, 5:44 PM.

Details

Reviewers

Yves

Commits

rP20572: Delete unused SpiderMonkey GarbageCollection ScriptFunctions ForceGC in the AI…

Summary

These function are unused.

Docs:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_MaybeGC
The information found here "should be ignored completely": https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Internals/Garbage_collection

Some history:
rP8622 introduced the ScriptInterface MaybeGC function and called it every 10th turn in CSimulation2Impl::Update
rP9220 exposed it to rmgen JS, but it was never called.
rP13225 was a major CCmpAIManager commit and added a number of MaybeGC calls and a ForceGC function there
rP14877 updated SM to v24 and

removed the CCmpAIManager MaybeGC
introduced MaybeIncrementalRuntimeGC and changed the simulation MaybeGC call to that
left MaybeGC unused

rP15787, rP15831, #2808 tuned the MaybeIncrementalRuntimeGC calls.

In current svn, we still have these MaybeIncrementalRuntimeGC and a number of calls to native SpiderMonkey functions:

JS::IsIncrementalGCEnabled
JS_GetGCParameter
JS::IsIncrementalGCInProgress
JS::IncrementalGCSlice
JS::StartIncrementalGC
JS_AddExtraGCRootsTracer
JS_RemoveExtraGCRootsTracer
JS_GC
...

As we can see from the revision history:

Optimizing the garbage collection isn't simple. Adding reasonable GC calls requires a good understanding of the GC, not too few code and lots of time.
Many more places than these two are possible use cases.
Many more functions than these two are often required to address the use cases.

So the unused functions don't seem to lower the effort for GC optimization and hence can be removed.

Test Plan

Read the summary, make sure that it compiles, note they are unused.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Nov 26 2017, 5:44 PM

Executing section Default...
Executing section Source...

source/ps/scripting/JSInterface_Debug.cpp
|  44| »   return·*(volatile·int*)0;
|    | [MAJOR] CPPCheckBear (nullPointer):
|    | Null pointer dereference
Executing section JS...

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Harbormaster completed remote builds in B3839: Diff 4403.Nov 26 2017, 8:58 PM

I have asked Yves and received agreement with regards to the the statement that changing Garbage Collection code needs to be done very carefully, thus removing the unused code doesn't lower the effort for GC optimization and hence can be removed and that I may quote him on this. He also posted relevant commits, forum links and an example how SM GC became easier in v31 following the removed need for TestingObjects.

Diese Funktionen können meiner Meinung nach weg.
GC hat sich von Spidermonkey 1.8.5 zu v24 und v31 komplett verändert. Wenn man irgend etwas daran ändert, muss man das so oder so sehr genau testen, Speichernutzung messen usw.
Also ist deine Aussage sicher richtig: "So the unused functions don't seem to lower the effort for GC optimization and hence can be removed."
Ich habe mal etwas im Forum geschrieben zu den Optimierungen: https://wildfiregames.com/forum/index.php?/topic/18466-spidermonkey-esr31-upgrade/&do=findComment&comment=300323
Das war für das Upgrade auf v31. Zu der Zeit musste man noch ein "TestingFunctions" Objekt verwenden um gcPreserveCode zu aktivieren. Heute geht das etwas einfacher: https://trac.wildfiregames.com/browser/ps/trunk/source/scriptinterface/ScriptInterface.cpp#L374
Für GC Anpassungen sind sicher diese beiden Commits noch interessant:
SM31 upgrade: rP16214
Anpassung an GC calls: rP15787

Of course that is considered insufficient to commit this patch.

Yves added a reviewer: Yves.Dec 3 2017, 11:19 AM

Yves added a subscriber: Yves.

As I said, changes to Garbage Collection need a lot of testing. These unused functions were left in during the upgrade, but I'm not sure if using them makes sense anywhere.
In such a situation i'd also vote for removing the code. If there's ever a situation where triggering GC manually from scripts is necessary, then it should be quite easy to add it back (and a bit of work for testing, but you have that anyway). Leaving it in is probably more confusing than helpful.

This revision is now accepted and ready to land.Dec 3 2017, 11:33 AM

Closed by commit rP20572: Delete unused SpiderMonkey GarbageCollection ScriptFunctions ForceGC in the AI… (authored by elexis). · Explain WhyDec 3 2017, 1:48 PM

This revision was automatically updated to reflect the committed changes.

Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Dec 3 2017, 1:48 PM

elexis mentioned this in rP21866: Move UserReporter C++/JS functions from JSInterface_Debug.cpp to….Aug 22 2018, 3:11 PM

elexis mentioned this in rP22555: Wipe MaybeGC/ForceGC declaration leftover from rP20572 / D1073..Jul 26 2019, 2:41 PM