00:04:14 <Waldo> mm, yeah
00:04:22 <Waldo> I think I am going to split this puppy up
00:04:26 <Waldo> ...tomorrow
00:04:44 <sfink> unfortunate metaphor
00:09:54 <pbone> good morning.
00:11:01 <pbone> sfink: pfft.  After my friend turned the 3-colour GC abstraction into a race thing, plus all the "evicting the nursery" etc metaphors we have I'm not sure puppy-slicing is all that bad.
00:11:32 <sfink> well, ok, I cannot deny your points
00:12:04 <pbone> I think we can agree there are many many unfortunate metaphors here.
00:15:44 <sfink> yes, I guess when we're depending on high infant mortality for things to work well, we can't exactly take the high road
00:15:56 <pbone> that's the one.. yes.
00:16:35 <Waldo> doubtful anything will ever beat http://quotes.burntelectrons.org/4823
00:16:37 * pbone usually doesn't spell it out..  not since he said it in a presentation to 100s of people.
00:17:03 <Waldo> aaaaaaaaaaaanyway
00:17:05 <Waldo> later all
00:32:56 <pbone> Doh,  forgot to "moz-phab submit" yesterday.
00:34:28 <sdetar> pbone: 1:1?
00:35:55 <pbone> Oh.
00:36:00 <pbone> damn. forgetting all my meetings.
00:36:11 <pbone> sdetar: I'm joining now.
01:36:29 <pbone> confession: Landing Bug 1569840
01:36:29 <confession> recorded, see https://mrgiggles.github.io/histoire/?user=pbone
01:36:30 <firebot> https://bugzil.la/1569840 — ASSIGNED, pbone⊙mc — Add a gcNurseryBytes runtime parameter
01:41:57 <pbone> sfink: Regarding your PTO, I already put you as a reviewer for something. If you don't have time feel free to re-assign it.
01:59:26 <pbone> jandem: regarding Bug 1568410
01:59:27 <firebot> https://bugzil.la/1568410 — ASSIGNED, pbone⊙mc — Make JS_DestroyContext to call Mutex::ShutDown()
01:59:38 <pbone> jandem: is calling this in JS_DestroyContext() safe?
02:00:00 <pbone> or do the helper threads need this to stay in their Main function.
02:00:14 <pbone> We were trying to move it to avoid repeating that for each thread's main function.
02:00:58 <pbone> jandem: Oh sorry, I thought your comment happend in the last few minutes, so assumed you were awake.
07:37:51 <jandem> pbone: posted a suggestion in phabricator. Curious what you think
08:05:28 <jandem> wingo: ping; not sure if you get phabricator email still but I requested review on a BigInt patch a few days ago :)
08:06:47 <wingo> jandem: hi!  tx for ping, i was out of office and just got back today
08:06:50 <wingo> i'll have a look :)
08:07:20 <jandem> wingo: ah, no rush, thanks
08:39:07 <pbone> jandem: okay thanks. I might take a look tonight.  If not Monday.  Thursday & Friday are PTO.
08:39:34 <jandem> pbone: sounds good. Enjoy your PTO
10:32:00 <jandem> jonco: ping
10:32:25 <jonco> jandem: hey
10:33:51 <jandem> jonco: hey. When we have a shutdown gc leak, do we still end up releasing those pages eventually or are they leaked too?
10:35:13 <jonco> jandem: good question, I think we still release the pages
10:35:53 <jandem> jonco: trying to decide what's the sanest thing to do for bug 1569655. I could force release those pools in debug builds to prevent more assertion failures later on in the process-wide allocator...
10:35:54 <firebot> https://bugzil.la/1569655 — NEW, nobody⊙mo — Don't crash debug Firefox builds when we're leaking ExecutablePool
10:36:11 <jandem> alternative is to add a process wide "leaked some runtime" flag
10:36:18 <jonco> yes we do free the pages
10:36:29 <jonco> jandem: I was looking at this a while ago
10:36:31 <jandem> the advantage there is that we can assert it's false in the shell, which is nice
10:37:14 <jonco> jandem: I was going to indicate to the leak checker that GC things leaked, and finalize them anyway
10:37:31 <jonco> which would make all our shutdown assertions pass even if there was a leak
10:38:01 <jandem> jonco: if we could finalize them anyway that would be really nice. These things are pretty annoying for sanity check asserts
10:38:20 <jonco> yes, this bothers me too
10:38:36 <jonco> bug 1407593
10:38:37 <firebot> https://bugzil.la/1407593 — NEW, jcoppeard⊙mc — Embedding leaks make it harder to assert shutdown correctness
10:38:49 <jonco> I guess I'll try and look at that again
10:42:28 <jandem> jonco: nice, it already has a patch. Do you think it's a matter of rebasing it or were there other issues? If the former maybe i should wait for that instead of doing anything about bug 1569655
10:44:06 <jonco> it was more work that than, however I think it's the best way forward for this problem
10:44:19 <jonco> I can take a look this afternoon
10:45:16 <jandem> great, thanks!
12:49:14 <wingo> is there a way to make the js shell wait until a promise is resolved, before exiting?
12:52:52 <jonco> wingo: does drainJobQueue() work? e.g. https://searchfox.org/mozilla-central/source/js/src/jit-test/tests/modules/bug-1501154.js#11
12:52:53 <firebot> https://bugzil.la/1501154 — FIXED, jcoppeard⊙mc — Assertion failure: throwing, at js/src/vm/JSContext.cpp:1449
12:53:41 <wingo> jonco: tx for the link
12:53:59 <wingo> that will probably work
12:54:14 <jonco> np
13:26:35 <arai> jonco: review ping for bug 1562102
13:26:36 <firebot> Bug https://bugzil.la/1562102 is not accessible
13:29:34 <jonco> arai: sorry, this didn't show up in my review queue
13:29:39 <jonco> I'll have a look
13:45:43 <jonco> arai: LGTM
13:45:51 <arai> thanks!
13:45:55 <jonco> arai: if I understand what's happening here, we usually allow creation of Handles of base types like this, horrible though it is: https://searchfox.org/mozilla-central/source/js/public/RootingAPI.h#1211-1218
14:03:53 <Waldo> the memory allocator bug is bug 1546442
14:03:54 <firebot> https://bugzil.la/1546442 — REOPENED, gpascutto⊙mc — Leading guard pages for normal allocations
14:17:09 <smaug> denispal: (and jonco) I wonder is ScriptLoader could use https://searchfox.org/mozilla-central/source/js/src/gc/GCRuntime.h#357 for now
14:19:14 <jonco> JS::IsIncrementalGCInProgress would be the public API for this
14:20:06 <smaug> ah, thanks
14:21:02 <smaug> jonco: ... just to not do offthemainthread stuff for now if we have GC running
14:22:34 <jonco> smaug: is this bug 1501608?
14:22:35 <firebot> https://bugzil.la/1501608 — NEW, dpalmeiro⊙mc — Investigate reducing size of ScriptSourceObject by moving DOM related fields outside the engine
14:23:41 <smaug> jonco: just a way to improve situation a bit before https://bugzilla.mozilla.org/show_bug.cgi?id=1543806 gets fixed
14:23:42 <firebot> Bug 1543806 — NEW, mgaudet⊙mc — Incremental GC can block off-thread parsing
14:24:28 <jorendorff> baseline interpreter not translating to mobile seems impossible
14:24:31 <jorendorff> what
14:27:17 <jonco> smaug: we only need to avoid this if we're collecting the atoms zone
14:27:47 <smaug> jonco: aha. Is there an API to check that already?
14:28:19 <jonco> smaug: denispal: the best place to try this is to change CanDoOffThread to always return false if OffThreadParsingMustWaitForGC here https://searchfox.org/mozilla-central/source/js/src/vm/OffThreadScriptCompilation.cpp#48-61
14:28:41 <smaug> right
14:29:02 <smaug> add some small tweak there to return false if we're collecting atoms zone
14:33:31 <jonco> smaug: the good news is that mgaudet is working on bug 1544117 which will fix this problem
14:33:33 <firebot> https://bugzil.la/1544117 — NEW, nobody⊙mo — [meta] Parse JS without allocating on the GC
14:34:07 <smaug> sure sure, this change would be just a small tweak before that all lands
14:34:49 <smaug> (mentioned during JS perf meeting)
14:34:58 <jonco> ok cool
14:38:08 <smaug> jonco: so one could just call https://searchfox.org/mozilla-central/source/js/src/vm/HelperThreads.cpp#809-816 I guess ?
14:39:29 <jonco> smaug: we do that, but we still allow off-thread compilation for large scripts
14:39:51 <jonco> smaug: I don't know if we ever tested what a suitable definition of 'large' was
14:40:05 <Waldo> this |this|-in-derived-constructors perf thing looks kind of silly
14:40:18 <jonco> smaug: anyway, it makes sense to disallow this for all scripts and see what happens
14:40:35 <smaug> oops, I wasn't reading all of  CanDoOffThread :)
14:40:40 <smaug> jonco: yup
14:40:43 <smaug> thanks
14:40:48 <Waldo> this is really basically identical to the problem of TDZ checks for lexical bindings, and we eliminate a lot of those via strict-domination checks
14:41:19 <Waldo> seems like we should be able to fit |this| into that framework somehow without insane difficulty
14:41:54 <Waldo> and just treat super() as doing the same thing as evaluating a lexical declaration does, in terms of disabling subsequent TDZ checks of the binding
14:45:53 <jorendorff> Waldo: agree
14:47:30 <smaug> jonco: and silly me, I apparently have pushed https://hg.mozilla.org/try/rev/bc3d5ce21c69c5a270d488d1dbe8c37ae62337b6 once to tryserver
14:48:17 <jonco> heh
14:50:18 <denispal> smaug: did you see any improvements?
14:50:29 <smaug> denispal: looks like I have lost the data
14:50:34 <smaug> I'm just pushing that again
14:50:45 <denispal> smaug: ah ok
14:52:42 <denispal> jonco:  ping.  I am hitting one issue with the CC in bug 1501608, was wondering if you maybe had an idea of what may be going wrong
14:52:44 <firebot> https://bugzil.la/1501608 — NEW, dpalmeiro⊙mc — Investigate reducing size of ScriptSourceObject by moving DOM related fields outside the engine
14:53:17 <denispal> jonco: when I create a LoadedScript in the EventListenerManager and then later use the callback to get the element from it, the CC has removed the mFetchOptions element and set it to null
14:54:49 <denispal> seems to only reproduce on the try server for 1-2 tests, but not sure if I'm maybe doing something wrong.   we do call HostAddRefTopLevelScript() on the LoadedScript, but is there something stronger I would need?
14:55:16 <jonco> denispal: is the LoadedScript being freed then?
14:55:31 <jonco> HostAddRefTopLevelScript should keep it alive
14:58:18 <denispal> jonco:   It doesn't seem like HostReleaseTopLevelScript is ever called for those scripts, so I wonder if it's being freed somewhere else?
14:58:36 <denispal> not up to the crash at least
15:01:52 <jonco> denispal: the patch looks fine
15:03:04 <smaug> jonco: FWIW, I didn't quite understand memory management in general
15:03:35 <smaug> is it not possible to have cycle SSO -> LoadedScript ->... something something ... SSO
15:05:29 <jonco> smaug: the cycle collector knows about the SSO -> LoadedScript edge and can collect those cycles
15:06:19 <smaug> ok, great. I just didn't see where that is handled 
15:06:26 <tcampbell> Waldo: yeah, that makes sense
15:07:28 <jonco> smaug: it's here in CycleCollectedJSRuntime::NoteGCThingXPCOMChildren https://searchfox.org/mozilla-central/source/xpcom/base/CycleCollectedJSRuntime.cpp#677-680
15:07:49 <smaug> aha
15:07:51 <smaug> thanks
15:08:21 <smaug> I was looking for JSCLASS_PRIVATE_IS_NSISUPPORTS usage or such
15:08:40 <jonco> smaug: it wasn't possible to reuse that for this case unfortunately
15:08:44 <jonco> there's a bug on file to clean this up
15:08:53 <smaug> yeah
15:10:01 <jonco> denispal: I take it ScriptSourceObject::setPrivate is getting called and calling the AddRef hook?
15:10:26 <jonco> denispal: it seems strange that this would get collected…
15:11:33 <denispal> jonco: yeah I did check that AddRef is being called
15:11:45 <jonco> cool
15:13:23 <denispal> very bizarre, I can't reproduce it on any machine locally
15:13:42 <denispal> only on try, and very consistently
15:15:00 <denispal> I will keep hacking away at it though, should figure it out eventually
15:15:12 <jonco> ugh, annoying
15:15:45 <mgaudet> denispal: what platform?
15:16:11 <denispal> mgaudet: It's pretty consistent on Windows, and 70% reproducible on Linux
15:16:59 <mgaudet> denispal: You should email b.holley, and get pernosco access -- this sounds like a case right up its alley
15:17:28 <denispal> mgaudet: I did :)  Although it doesn't seem like pernosco can reproduce it either
15:17:35 <mgaudet> denispal: Darn
15:17:52 <denispal> it sends me emails about other intermittent failures, but never these specific tests unfortunately
15:19:34 <mgaudet> :( 
15:51:00 <Waldo> mrgiggles: pun
15:51:00 <mrgiggles> Waldo: Some Spanish government employees are Seville servants.
16:22:30 * Waldo mutters
16:22:45 <Waldo> paging in the parser and frontend and name analysis and how names get declared and used, what fun
17:05:06 <tcampbell> sfink: ping?
17:05:57 <tcampbell> how does AutoKeepAtoms actually work when you release it
17:06:54 <tcampbell> like, we use it the frontend before we start handing around naked JSAtom pointers, but those get stored in GC thing
17:07:22 <tcampbell> presumably we can release the AutoKeepAtoms during an incremental-gc
17:07:40 <jonco> tcampbell: it only does anything when it's live
17:07:45 <jonco> (sfink is on PTO I think)
17:08:18 <tcampbell> so there needs to be a final markAtoms before releasing it?
17:09:01 <jonco> tcampbell: no
17:09:31 <tcampbell> Is this where traceKeptAtoms kicks in?
17:09:45 <jonco> tcampbell: all the atoms you've have must be written into GC things before AKA dies
17:10:32 <jonco> tcampbell: that is called during root marking
17:11:13 <jonco> atoms that are created after root marking during an incremental GC will end up marked automatically, by the normal rules for an incremental GC
17:11:28 <tcampbell> ... starting to make sense
17:11:46 <jonco> the problem is only when we mark the roots and don't know which atoms the parser has live in the parse tree
17:13:37 <tcampbell> so the atomCache are the roots for that case?
17:13:46 <tcampbell> while the ASA is live
17:13:56 <jonco> yes we mark everything in the zone's atom cache in this case
17:14:57 <tcampbell> cool. The source of my questions is that we want to call Handle::fromMarkedLocation on a JSAtom* generated in parser while both are under ASA
17:15:14 <tcampbell> *both the atomization and the fromMarkedLocation are under ASA
17:15:28 <tcampbell> I *think* that is all fine
17:16:30 <jonco> please avoid fromMarkedLocation if at all possible
17:16:53 <jonco> yes, this sounds like it's fine
17:18:13 <tcampbell> the future is that we will eventually make parser atoms different than JSAtom
17:18:25 <tcampbell> which will get rid of the whole ASA mess entirely
17:18:36 <jonco> that would be great
17:19:06 <jonco> in the mean time is it possible to root the atom?
17:19:51 <tcampbell> jonco: see: https://phabricator.services.mozilla.com/D39771 the 'FunctionCreationData'
17:20:11 <jonco> (I guess not or you wouldn't ask, but…)
17:20:15 <tcampbell> were just trying to avoid having to trace and root the temporary now
17:21:39 <tcampbell> let me look at how exactly it is used at the end of patch series
17:25:26 <tcampbell> jonco: is a Handle<T*> any slower than a T* for non-gc types?
17:25:40 <tcampbell> (A Handle<Cell*> is a Cell**, right?)
17:25:52 <jonco> tcampbell: yeah Handle<T> is basically T*
17:26:24 <tcampbell> ah right
17:27:43 <tcampbell> okay, I'm tempted to use the fromMarkedLocation for the first landing, but try and make it rooted pretty soon
17:27:51 <tcampbell> lots of moving pieces to juggle =\
17:29:27 <jonco> can you enforce that you are always under AutoKeepAtoms somehow?
17:30:01 <tcampbell> assert immediately before it
17:30:25 <tcampbell> can make it a diagnostic assert even
17:30:37 <jonco> the use of 'fromMarkedLocation' when it's used on a location that is not marked explicitly kind of grates…
17:30:57 <tcampbell> hmm
17:30:59 <jonco> tcampbell: also assert in the struct's destructor to check it doesn't get released?
17:31:46 <tcampbell> at that point I need to plumb in the cx stuff and should just fix the root immediately
17:32:29 <jonco> use TLS in the assert?
17:32:53 <tcampbell> interesting
17:33:19 <tcampbell> let me redownload the stack and look at what just fixing the rooting will do to the stack
17:41:37 <tcampbell> jonco: so I should use a HeapPtr<JSAtom>?
17:43:19 <jonco> tcampbell: this is heap allocated, right? if you're going to trace it as a root then JSAtom* is fine
17:43:49 <tcampbell> eventually heap allocated and rooted in the Parser
17:44:02 <tcampbell> okay, will do
17:44:14 <jonco> great
17:44:25 <tcampbell> that mostly avoids the cx then
17:44:44 <jonco> yep
17:45:03 <jonco> feel free to tag me for review if you like
17:45:37 <tcampbell> jonco: thanks. I'm going put together a patch in the middle of stack that replaces the fromMutableHandle with tracing and I will flag you.
17:46:36 <jonco> great, thanks for doing this
17:54:23 <tcampbell> jonco: so I have a Handle<FunctionCreateData>.. to get a HandleAtom out of the jsatom* field, I still need frommarkedlocation, correct?
17:56:10 <jonco> tcampbell: yes (but it will really be from a marked location now)
18:05:06 <tcampbell> mgaudet: let me know when you have some time to go through this stack again
18:05:36 <mgaudet> tcampbell: I'm just working through feedback as we speak; so, anytime now. 
18:05:55 <tcampbell> mgaudet: can we do a call in 5 min?
18:06:13 <mgaudet> Sure; gives me just enough time to sneak up and make a coffee
18:06:17 <tcampbell> :)
18:06:25 <mgaudet> tcampbell: will ping on return
18:10:36 <mgaudet> tcampbell: just tell me where :) 
18:55:55 <mgaudet> tcampbell: Just an IRC review of the hasGuessedAtomChange to make sure I'm reading it right: 
18:56:01 <mgaudet> https://irccloud.mozilla.com/pastebin/zricuwtQ/hasGuessed
19:02:23 * Waldo mutters
19:03:05 <tcampbell> mgaudet: yeah, that looks right
19:03:38 <mgaudet> tcampbell: thanks for gut check. 
19:29:40 <Waldo> mrgiggles: pun
19:29:40 <mrgiggles> Waldo: I used to be a tap dancer until I fell in the sink.
19:30:43 <elexis> hello everyone!
19:30:43 <elexis> I'm one of the 0ad developers, the game uses SpiderMonkey. As such I ended up wondering whether JS::PersistentRooted is more, less or equal performant to JS::Heap (assuming that the object has a long lifetime)?
19:31:11 <elexis> JS::PersistentRooted leaves much cleaner code, so it seems that would be preferable https://pastebin.com/hz2J9wyz
19:32:08 <elexis> there are a bunch of JSObjects created each time the user opens a GUI page, and opening / closing GUI pages happens rarely, so the only performance relevant part would be the GC treatment
19:33:05 <tcampbell> elexis: I have a few notes on this in https://github.com/spidermonkey-embedders/spidermonkey-embedding-examples/blob/esr60/examples/tracing.cpp#L95
19:34:02 <tcampbell> the biggest risk of persistentrooted is usually memory leaks
19:34:05 <elexis> we had that problem too (one array with many JS objects vs one JS object with many items)
19:34:38 <elexis> if no JS object owns a C++ object, there can't be a leak, right?
19:34:47 <tcampbell> yep
19:34:50 <tcampbell> (I think)
19:34:51 <elexis> great!
19:35:26 <tcampbell> the persistentrooted moves means the GC takes longer for the root-marking stage, rather than spreading over incremental slices
19:35:40 <mgaudet> tcampbell: Like a dummy I just queued the first four patches for landing before actually making the FunctionBoxVector change; I'll get you a follow up patch for that right now. 
19:35:54 <tcampbell> mgaudet: lol.. I just noticed that
19:36:32 <mgaudet> tcampbell: autoland being closed makes me wish there was an "oops" button on lando 
19:37:12 <tcampbell> elexis: my general advice would be that avoiding the persistentrooted forces you to be a bit more disciplined about GC tracing
19:38:05 <elexis> right now by design no JS object owns a C++ object, so leaks arent of concern
19:38:35 <elexis> so I wonder why we want this Tracer code to begin with if we could use a PersistentRooted
19:38:55 <elexis> unless there is a performance reason
19:39:43 <tcampbell> if you have many objects, then a single Rooted<Foo> with an array of JS objects will be much faster than an array of PersistentRooted
19:40:00 <elexis> the situation is basically as follow: user clicks on a button -> GUI page opens -> maybe 100 GUI objects are created -> each GUI object is represented by one JS object -> user closes page -> JS objects deleted
19:42:04 <elexis> GUI Page stores GUI Objects (button, dropdown, text, ...), each GUI Object is a C++ class that owns one JS object, so can't really change that to be only one JS object without entering dangerous territory
19:42:37 <elexis> so the question is only: Is one JS::Heap object faster to keep track of for the GC than one JS::PersistentRooted object
19:43:39 <elexis> (not considering allocation and deallocation)
19:43:42 <tcampbell> marginally
19:45:21 <tcampbell> elexis: I'm skimming the code. What the the name of an example GUI type?
19:45:40 <tcampbell> oh, I see your pastebin
19:45:42 <elexis> every GUI object type inherits the IGUIObject class, which keeps track of all of that
19:46:15 <elexis> https://code.wildfiregames.com/source/0ad/browse/ps/trunk/source/gui/IGUIObject.cpp
19:46:52 <elexis> user opens a GUI page -> JS object is created -> 30 seconds pass -> user closes page
19:47:11 <elexis> so the few microseconds upon opening / closing the page is not so relevant, more what happens during the 30 seconds
19:48:23 <tcampbell> ah, so for that lifetime, the GCs will be marginally slower (but not really noticeable if you have ~100 objects)
19:48:32 <elexis> I suppose there are < 500 objects tops on the ingame GUI page
19:48:50 <tcampbell> PersistentRooted is definitely an improvement over JS_RemoveExtraGCRootsTracer
19:49:55 <elexis> depending on whether theres a significant performance difference, it'd be the recommended default for future patches, and the codebase would probably be changed for one or the other
19:50:11 <elexis> the code definitely looks nicer with PersistentRooted
19:50:17 <tcampbell> so the idea of using Heap over PersistentRooted, is that you would have a single PersistentRooted for the top-level, and then your GUI would know have to traverse the tree
19:50:43 <tcampbell> and then trace the Heap that hang off each GUI node
19:51:00 <tcampbell> if there isn't already a GUI tree relation, that might be ackward though
19:51:04 <tcampbell> *awkward
19:52:00 <elexis> there is a tree, but it's C++
19:52:10 <tcampbell> that is fine
19:52:23 <tcampbell> the trace methods are written in C++ in your code
19:52:53 <tcampbell> eg. https://github.com/spidermonkey-embedders/spidermonkey-embedding-examples/blob/esr60/examples/tracing.cpp#L32
19:53:38 <tcampbell> that is the recommended pattern that spidermonkey and gecko try to follow
19:55:04 <tcampbell> mgaudet: just stack another patch for the rename FunctionBoxVector and I can stamp it
19:55:38 <mgaudet> tcampbell: working on it -- took a couple extra minutes while I puzzled through what I wanted to do with the existing FunctionBoxVector type :P 
19:55:45 <tcampbell> ah, got it
19:56:25 <tcampbell> hmm.. I didn't realize that existed
19:56:45 <mgaudet> tcampbell: Neither did I 
19:57:03 <mgaudet> so I sunk it to its use, (nice for shortening the dependent declaration, but doesn't need huge scope like it has) 
19:57:15 <mgaudet> and have declared our function box type near AtomVector
19:57:59 <tcampbell> nice
19:58:43 <tcampbell> elexis: do you happen to know what version of spidermonkey the codebase is on?
19:58:53 <elexis> 38, but we're working on it :->
19:59:00 <tcampbell> cool
19:59:20 <elexis> (45 patch for review, 52 in some branch, and we wont stop there by the looks of things)
19:59:47 <mgaudet> \o/
20:00:05 <tcampbell> nice
20:00:52 <elexis> performance improvements will be more relevant for the ingame simulation, with thousands of units walking around, each having 10-20 JS objects representing their components
20:01:55 <elexis> I just wanted to know whether I'm adding a performance regression for the GUI when I change the JS::Heap to JS::PersistentRooted, but if its not signfiicant for < 500 objects that are only rooted / unrooted once every 10ish seconds, then it should be fine
20:02:30 <elexis> in theory I thought JS::PersistentRooted would be faster, because the GC could always ignore it until its destroyed
20:03:31 <tcampbell> persistentrooted means the gc has to check it every time during the beginning of a GC, and can't spread it over incremental
20:03:43 <tcampbell> every edge still needs to be tested during the GC process
20:04:10 <tcampbell> but the GC can split the work over mutliple slices that interleave with application work 
20:05:20 <elexis> I see
20:05:44 <elexis> so the same amount of work, just not as well spread
20:06:47 <tcampbell> elexis: so one example in your pastebin. With  m_ScriptHandlers, it would be preferred to have the map contain Heap<T> and then you could have a single PersistentRooted around your this pointer. Then implement the little trace method that looks at the map.
20:07:02 <tcampbell> that would reduce the number of roots
20:13:54 <elexis> so only one PeristentRooted globally, hence the GC only has to iterate over one GC thing, but in order to access one of the items, one has to resolve / unwrap that PersistentRooted again, which also adds overhead, doesnt it?
20:14:29 <elexis> std::map<string, JSObject> could be changed to a JSObject containing the JSObjects however
20:14:50 <elexis> that might be less complex than introducing a wrapper class
20:15:27 <elexis> (did I get that right?)
20:23:55 <elexis> if the trace method is needed, then why add the PersistentRooted pointer
20:24:17 <elexis> only advantage would be spreading GC workload over multiple slices?
20:24:51 <elexis> its currently already Heap, so I guess one doesnt need another persistentrooted container
20:29:10 <gkw> sfink: bug 1570465 - bisecting now but I think this is yours
20:29:12 <firebot> Bug https://bugzil.la/1570465 is not accessible
20:35:55 <Waldo> gkw: allegedly he's on PTO, the scoundrel
20:36:05 <gkw> lol
20:39:39 <tcampbell> elexis: so one has to start roots somewhere. A few of the ways one can do that are JS::PersistentRooted, using a Rooted<T> on stack, using the (deprecated) JS_RemoveExtraGCRootsTracer 
20:40:01 <tcampbell> generally you want to minimize the number of them and minimize how often they are come and go
20:40:41 <tcampbell> those roots then point to an arbitrary number of objects in some sort of C++ graph that is your data structures
20:41:44 <tcampbell> we use Heap<T> (and inside spidermonkey GCPtr which is fast but less safe) to connect up the graph. The GC can then traverse the graph in slices and split up the work and avoid repetition in some cases
20:44:52 <mgaudet> elexis: (If you haven't seen this, https://github.com/spidermonkey-embedders/spidermonkey-embedding-examples/blob/esr60/docs/GC%20Rooting%20Guide.md, some of the rooting reasoning is covered in there too) 
20:46:41 <mgaudet> tcampbell: I'm feeling slow; if I have a Rooted<FunctionCreationData>, and I pass and |Handle<FnctionCreationData> data|, what magic am I missing to make it so |data.<member>| works without needing to do |data.get().<member>| 
20:47:48 <tcampbell> oh right.. I ran into that :(.. I really hope the answer isn't that we need WrappedPtrOperations
20:48:33 <mgaudet> Handle<PropertyDescriptor> works; not sure why it's special? https://searchfox.org/mozilla-central/source/js/src/vm/ArgumentsObject.cpp#672,689 
20:49:04 <tcampbell> mgaudet: https://searchfox.org/mozilla-central/source/js/public/PropertyDescriptor.h#278
20:49:06 <tcampbell> =\
20:49:08 <mgaudet> oh dear. 
20:49:18 <tcampbell> yeah.... bleh
20:49:37 <tcampbell> I know if you have Rooted<UniquePtr<T>> you get sane behaviour
20:50:19 <mgaudet> hrm. Heap allocation, or some extra .get() calls? 
20:50:33 <tcampbell> the eternal question
20:50:57 <tcampbell> I.. don't know =\\
20:51:05 <mgaudet> I"m leaning get()
20:51:15 <mgaudet> but we'll see what you say when you see it in-situ
20:51:22 <tcampbell> That will get() the job done
20:52:56 <mgaudet> I suppose a third option is to have a const FunctionCreationData& local to the function; which seems even nicer
20:58:41 <tcampbell> not the worst. We know the FunctionCreationData aint moving
21:05:04 <elexis> I didn't know one can use JS::Rooted<T> for T that isn't a GC thing but a class that provides a trace method
21:06:52 <tcampbell> yeah, still only on stack though
21:09:39 <elexis> but the GC process still has to iterate over everything, it just can spread it better?
21:10:32 <elexis> only the process of rooting every single item falls away?
21:10:40 <tcampbell> correct
21:10:58 <tcampbell> spidermonkey has to worry about keeping memory leaks quite a bit, so we avoid persistentrooted
21:11:09 <elexis> sure
21:11:31 <elexis> SM has to work with all applications in all circumstances
21:11:47 <elexis> i.e. under the highest workload too
21:11:50 <tcampbell> there is a minor thing that with one root you cross the library boundary less which is faster
21:13:30 <tcampbell> there are a bunch of little optimizations that the GC team has done about when we need virtual calls and when we don't. I'm not really sure about the precise tradeoffs
21:17:24 <mgaudet> confession: Reworking GC management in patch stack. how to root/handle tree is now tomorrow-Matthew's problem. 
21:17:24 <confession> recorded, see https://mrgiggles.github.io/histoire/?user=mgaudet
21:19:17 <mgaudet> confession: Landed preq patches for allocation deferral 
21:19:17 <confession> ok, got it
21:19:51 <elexis> then the paste is a bad idea because the PersistentRootedObject will consume a little more time (rooting as opposed to not rooting when using heap) - so much time that it would be significant if it was an order of magnitude more objects (5000?), thus setting a bad precedent, even if memory leaks are of no concern
21:25:24 <elexis> the IGUIObject class could be Rooted, sounds only a little bit like a monster
21:26:31 <tcampbell> elexis: yeah, I think I'd agree with both of those
21:26:46 <elexis> (I guess not the CGUI class, since that defines its own JSContext / JSRuntime to operate in)
21:26:54 <elexis> (which is one instance per GUI page)
21:29:38 <elexis> thank you a lot for taking the time and nerves to explain that tcampbell!
21:30:38 <elexis> I will cleanup those classes that have almost not been touched since 2004, and I'll see where it can be taken with new SM technology :->
21:31:35 <tcampbell> :)
21:35:22 <tcampbell> mccr8: did we decide if having a NukeNonCCWProxy was a good or bad idea? I'm trying to figure out the state of your patch
21:36:28 <mccr8> tcampbell: I wrote it!
21:36:41 <mccr8> tcampbell: it actually turns out to be useful for the Xray waiver issue I was having.
21:36:56 <tcampbell> ah, cool
21:36:58 <mccr8> well, I don't need the finalize part of it.
21:37:43 <mccr8> tcampbell: I got distracted with other stuff for a few days, but I'm back on this again. I'm going to put a patch up with the nuke non CCW thing and the waiver fix in bug 1570484.
21:37:44 <firebot> https://bugzil.la/1570484 — NEW, continuation⊙gc — Nuke Xray waivers for remote outer window proxies
21:38:09 <tcampbell> that's fine. I was just making sure I wasn't a bottleneck on reviews
21:46:44 <sstangl> confession: Made a start on threading jumps as part of the relaxation pass.
21:46:44 <confession> recorded, see https://mrgiggles.github.io/histoire/?user=sstangl