Page MenuHomeWildfire Games

Use JS::Heap instead of JS::PersistentRooted in IGUIObject
Changes PlannedPublic

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 (Update: fake news, there is no leak).

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 (Update: wrong, automatic unrooting upon destruction, i.e. on close page).
Other than that, JS::Heap also seems better practice than PersistentRooting (Update: only when initialization / rooting and unrooting is considered).

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.Dec 21 2018, 5:50 PM
wraitii accepted this revision.Dec 31 2018, 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.Dec 31 2018, 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.

Itms added a comment.May 26 2019, 2:23 PM

Regardless of the future merge between runtimes and context, I think we should use persistent rooteds as little as possible, so conceptually I agree with the patch.

Stan added a subscriber: Stan.May 26 2019, 2:25 PM

Would be nice to have the Valgrind report :)

wraitii resigned from this revision.Jul 31 2019, 9:05 PM
This revision now requires review to proceed.Jul 31 2019, 9:05 PM
elexis retitled this revision from Use JS::Heap instead of JS::PersistentRooted to fix IGUIObject memory leak in rP5154 to Use JS::Heap instead of JS::PersistentRooted in IGUIObject.Aug 2 2019, 2:01 AM
elexis edited the summary of this revision. (Show Details)
elexis edited the test plan for this revision. (Show Details)

Based on new documentation we've had 2 days ago on IRC, I think this would better wait SM45 and use JS::TraceEdge instead, refactoring the thing a little more, but it's a step in the right direction.

elexis added a comment.Aug 2 2019, 1:30 PM

1. There is no leak:

So first step of the review of this patch should be whether it's most basic assumptions are true.

It says "fix a leak", but there is no leak here, because the objects are destroyed when the IGUIObject is destroyed, and with it the JS Value is unrooted.

As cited in the preceding comment in https://code.wildfiregames.com/rP5154#inline-2592

There is "automatic unrooting on destruction" https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS::PersistentRooted

Even the code in that commit doesn't seem to leak memory, because there is a JS_AddRoot and JS_RemoveRoot call.

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_AddRoot
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_RemoveRoot

See also somewhat long discussion with wraitii on #0ad-dev:
http://irclogs.wildfiregames.com/2019-07/2019-07-31-QuakeNet-%230ad-dev.log

17:48 < wraitii> but yeah D1700 -> not fixing a leak

2. Alternative not have these objects hang around forever using up memory interpretation:

I can only imagine that Philip meant that the JS objects are not necessarily needed after they were accessed once, but they are not unrooted until the GUI page is closed, whereas before this commit, they were destroyed immediately after each access.

only a single JS_RemoveRoot is required to unroot the location.

So

objects hang around forever using up memory

could at best have meant

objects hang around for the lifetime of the GUI page using up memory

and that interpretation makes most sense to me, because Philip added the rooting code, so he should be aware, and the only behavior change in this commit is that the JSObjects exist for the GUIPage lifetime instead of the access request time.

3. To PersistentRooted or to JS::Heap

In D1700#79363, @Itms wrote:

Regardless of the future merge between runtimes and context, I think we should use persistent rooteds as little as possible, so conceptually I agree with the patch.

I'm not sure anymore of this, because PersistentRooting seems much cleaner, as it does avoid the Tracer code altogether.

4. Talk to SM devs

I joined #jsapi and asked the developers directly whether there is a significant performance difference.
https://mozilla.logbot.info/jsapi/20190731

Thanks a lot to tcampbell again for answering all of my questions patiently!

The results of the discussion are:

  • PersistentRooted and JS::Heap should have the same performance for the GC tracing. (The only difference is that PersistentRooteds can't be processed partially during GC slices.)
  • JS::Heap is faster with regards to allocation and deallocation, because it doesn't have to be rooted in a context

Considering that there are maybe 300 GUI objects per page, and maybe 100 event handlers, the rooting might not be significant.
But it would be preferable to chose the pattern that will be more scalable.

Therefore it should be JS::Heap.

5. Future of Rooting

tcampbell has hinted us at a new means to avoid rooting of JS values.
A C++ class only needs a trace function and then the GC can iterate through that if that was rooted in a context.
This means the Add and Remove Tracer calls can be removed in the future, leaving nicer code without PersistentRooted.

https://github.com/spidermonkey-embedders/spidermonkey-embedding-examples/blob/esr60/examples/tracing.cpp

	struct SafeBox {
	  JS::Heap<JS::Value> stashed;
          ....

	  void trace(JSTracer* trc) {
		  JS_CallValueTracer(trc, &stashed, "stashed value");
                  ...
	  }
	};

        // Only one root for many JS values!
	JS::Rooted<SafeBox> stackSafe(cx);

5. Test with SM45

This piece of code doesnt work with SM38 nor SM45.

So the proposed solution is to update SpiderMonkey before doing anything at all.

elexis planned changes to this revision.Aug 2 2019, 1:31 PM