Page MenuHomeWildfire Games

Use JS::Heap instead of JS::PersistentRooted to fix IGUIObject memory leak in rP5154
AcceptedPublic

Authored by elexis on Dec 19 2018, 4:18 PM.

Details

Reviewers
wraitii
Trac Tickets
#5369
Summary

Philip added and documented a memory leak in rP5154.
At least with SM38, the JS value representing the GUI object is persistently rooted, and PersistentRooting means it's registered with the ScriptRuntime rather than the JSContext.
But GUI Pages don't have a custom ScriptRuntime, only a JSContext, so these JSObjects will remain allocated forever.
Other than that, JS::Heap also seems better practice than PersistentRooting.

The patch uses a JS::Value instead of a JSObject to distinguish initialized from uninitialized cache.

Test Plan

It would be nice to measure the memory leak with a tool like valgrind+massif, but I failed to achieve that (https://code.wildfiregames.com/rP5154#inline-2592).
Even with memory allocation measuring (using ps) I could not notice an increase in allocated pages at all, with millions of registered objects.
Read the spidermonkey documentation.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6514
Build 10774: Vulcan BuildJenkins
Build 10773: arc lint + arc unit

Event Timeline

elexis created this revision.Dec 19 2018, 4:18 PM
Vulcan added a subscriber: Vulcan.Dec 19 2018, 4:35 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/828/

elexis updated the Trac tickets for this revision.Fri, Dec 21, 5:50 PM
wraitii accepted this revision.Mon, Dec 31, 9:54 AM
wraitii added a subscriber: wraitii.

Upstream SM do away with the context/runtime difference, but I think the change is fine regardless.

This revision is now accepted and ready to land.Mon, Dec 31, 9:54 AM

I didn't expect SM upgrades anytime soon when I wrote the patch.

The code looks nicer when using a JS::Object than a JS::Value I think, as we don't need to convert between val<->obj.
So maybe we can reevaluate this when the SM upgrades are through.