mm, yeah

I think I am going to split this puppy up

...tomorrow

unfortunate metaphor

good morning.

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.

well, ok, I cannot deny your points

I think we can agree there are many many unfortunate metaphors here.

yes, I guess when we're depending on high infant mortality for things to work well, we can't exactly take the high road

that's the one.. yes.

doubtful anything will ever beat quotes.burntelectrons.org/4823

aaaaaaaaaaaanyway

later all

Doh, forgot to "moz-phab submit" yesterday.

pbone: 1:1?

Oh.

damn. forgetting all my meetings.

sdetar: I'm joining now.

confession: Landing Bug 1569840

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.

jandem: regarding Bug 1568410

jandem: is calling this in JS_DestroyContext() safe?

or do the helper threads need this to stay in their Main function.

We were trying to move it to avoid repeating that for each thread's main function.

jandem: Oh sorry, I thought your comment happend in the last few minutes, so assumed you were awake.

pbone: posted a suggestion in phabricator. Curious what you think

wingo: ping; not sure if you get phabricator email still but I requested review on a BigInt patch a few days ago :)

jandem: hi! tx for ping, i was out of office and just got back today

i'll have a look :)

wingo: ah, no rush, thanks

jandem: okay thanks. I might take a look tonight. If not Monday. Thursday & Friday are PTO.

pbone: sounds good. Enjoy your PTO

jonco: ping

jandem: hey

jonco: hey. When we have a shutdown gc leak, do we still end up releasing those pages eventually or are they leaked too?

jandem: good question, I think we still release the pages

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...

alternative is to add a process wide "leaked some runtime" flag

yes we do free the pages

jandem: I was looking at this a while ago

the advantage there is that we can assert it's false in the shell, which is nice

jandem: I was going to indicate to the leak checker that GC things leaked, and finalize them anyway

which would make all our shutdown assertions pass even if there was a leak

jonco: if we could finalize them anyway that would be really nice. These things are pretty annoying for sanity check asserts

yes, this bothers me too

I guess I'll try and look at that again

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

it was more work that than, however I think it's the best way forward for this problem

I can take a look this afternoon

great, thanks!

is there a way to make the js shell wait until a promise is resolved, before exiting?

wingo: does drainJobQueue() work? e.g. searchfox.org/mozilla-central/sourc…est/tests/modules/bug-1501154.js#11

jonco: tx for the link

that will probably work

np

jonco: review ping for bug 1562102

arai: sorry, this didn't show up in my review queue

I'll have a look

arai: LGTM

thanks!

arai: if I understand what's happening here, we usually allow creation of Handles of base types like this, horrible though it is: searchfox.org/mozilla-central/source/js/public/RootingAPI.h#1211-1218

the memory allocator bug is bug 1546442

denispal: (and jonco) I wonder is ScriptLoader could use searchfox.org/mozilla-central/source/js/src/gc/GCRuntime.h#357 for now

JS::IsIncrementalGCInProgress would be the public API for this

ah, thanks

jonco: ... just to not do offthemainthread stuff for now if we have GC running

smaug: is this bug 1501608?

jonco: just a way to improve situation a bit before bugzilla.mozilla.org/1543806 gets fixed

baseline interpreter not translating to mobile seems impossible

what

smaug: we only need to avoid this if we're collecting the atoms zone

jonco: aha. Is there an API to check that already?

smaug: denispal: the best place to try this is to change CanDoOffThread to always return false if OffThreadParsingMustWaitForGC here searchfox.org/mozilla-central/sourc…ffThreadScriptCompilation.cpp#48-61

right

add some small tweak there to return false if we're collecting atoms zone

smaug: the good news is that mgaudet is working on bug 1544117 which will fix this problem

sure sure, this change would be just a small tweak before that all lands

(mentioned during JS perf meeting)

ok cool

jonco: so one could just call searchfox.org/mozilla-central/sourc…js/src/vm/HelperThreads.cpp#809-816 I guess ?

smaug: we do that, but we still allow off-thread compilation for large scripts

smaug: I don't know if we ever tested what a suitable definition of 'large' was

this |this|-in-derived-constructors perf thing looks kind of silly

smaug: anyway, it makes sense to disallow this for all scripts and see what happens

oops, I wasn't reading all of CanDoOffThread :)

jonco: yup

thanks

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

seems like we should be able to fit |this| into that framework somehow without insane difficulty

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

Waldo: agree

jonco: and silly me, I apparently have pushed try/bc3d5ce21c69c5a270d488d1dbe8c37ae62337b6 once to tryserver

heh

smaug: did you see any improvements?

denispal: looks like I have lost the data

I'm just pushing that again

smaug: ah ok

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

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

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?

denispal: is the LoadedScript being freed then?

HostAddRefTopLevelScript should keep it alive

jonco: It doesn't seem like HostReleaseTopLevelScript is ever called for those scripts, so I wonder if it's being freed somewhere else?

not up to the crash at least

denispal: the patch looks fine

jonco: FWIW, I didn't quite understand memory management in general

is it not possible to have cycle SSO -> LoadedScript ->... something something ... SSO

smaug: the cycle collector knows about the SSO -> LoadedScript edge and can collect those cycles

ok, great. I just didn't see where that is handled

Waldo: yeah, that makes sense

smaug: it's here in CycleCollectedJSRuntime::NoteGCThingXPCOMChildren searchfox.org/mozilla-central/sourc…CycleCollectedJSRuntime.cpp#677-680

aha

thanks

I was looking for JSCLASS_PRIVATE_IS_NSISUPPORTS usage or such

smaug: it wasn't possible to reuse that for this case unfortunately

there's a bug on file to clean this up

yeah

denispal: I take it ScriptSourceObject::setPrivate is getting called and calling the AddRef hook?

denispal: it seems strange that this would get collected…

jonco: yeah I did check that AddRef is being called

cool

very bizarre, I can't reproduce it on any machine locally

only on try, and very consistently

I will keep hacking away at it though, should figure it out eventually

ugh, annoying

denispal: what platform?

mgaudet: It's pretty consistent on Windows, and 70% reproducible on Linux

denispal: You should email b.holley, and get pernosco access -- this sounds like a case right up its alley

mgaudet: I did :) Although it doesn't seem like pernosco can reproduce it either

denispal: Darn

it sends me emails about other intermittent failures, but never these specific tests unfortunately

:(

mrgiggles: pun

paging in the parser and frontend and name analysis and how names get declared and used, what fun

sfink: ping?

how does AutoKeepAtoms actually work when you release it

like, we use it the frontend before we start handing around naked JSAtom pointers, but those get stored in GC thing

presumably we can release the AutoKeepAtoms during an incremental-gc

tcampbell: it only does anything when it's live

(sfink is on PTO I think)

so there needs to be a final markAtoms before releasing it?

tcampbell: no

Is this where traceKeptAtoms kicks in?

tcampbell: all the atoms you've have must be written into GC things before AKA dies

tcampbell: that is called during root marking

atoms that are created after root marking during an incremental GC will end up marked automatically, by the normal rules for an incremental GC

... starting to make sense

the problem is only when we mark the roots and don't know which atoms the parser has live in the parse tree

so the atomCache are the roots for that case?

while the ASA is live

yes we mark everything in the zone's atom cache in this case

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

*both the atomization and the fromMarkedLocation are under ASA

I *think* that is all fine

please avoid fromMarkedLocation if at all possible

yes, this sounds like it's fine

the future is that we will eventually make parser atoms different than JSAtom

which will get rid of the whole ASA mess entirely

that would be great

in the mean time is it possible to root the atom?

jonco: see: phabricator.services.mozilla.com/D39771 the 'FunctionCreationData'

(I guess not or you wouldn't ask, but…)

were just trying to avoid having to trace and root the temporary now

let me look at how exactly it is used at the end of patch series

jonco: is a Handle<T*> any slower than a T* for non-gc types?

(A Handle<Cell*> is a Cell**, right?)

tcampbell: yeah Handle<T> is basically T*

ah right

okay, I'm tempted to use the fromMarkedLocation for the first landing, but try and make it rooted pretty soon

lots of moving pieces to juggle =\

can you enforce that you are always under AutoKeepAtoms somehow?

assert immediately before it

can make it a diagnostic assert even

the use of 'fromMarkedLocation' when it's used on a location that is not marked explicitly kind of grates…

hmm

tcampbell: also assert in the struct's destructor to check it doesn't get released?

at that point I need to plumb in the cx stuff and should just fix the root immediately

use TLS in the assert?

interesting

let me redownload the stack and look at what just fixing the rooting will do to the stack

jonco: so I should use a HeapPtr<JSAtom>?

tcampbell: this is heap allocated, right? if you're going to trace it as a root then JSAtom* is fine

eventually heap allocated and rooted in the Parser

okay, will do

great

that mostly avoids the cx then

yep

feel free to tag me for review if you like

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.

great, thanks for doing this

jonco: so I have a Handle<FunctionCreateData>.. to get a HandleAtom out of the jsatom* field, I still need frommarkedlocation, correct?

tcampbell: yes (but it will really be from a marked location now)

mgaudet: let me know when you have some time to go through this stack again

tcampbell: I'm just working through feedback as we speak; so, anytime now.

mgaudet: can we do a call in 5 min?

Sure; gives me just enough time to sneak up and make a coffee

:)

tcampbell: will ping on return

tcampbell: just tell me where :)

tcampbell: Just an IRC review of the hasGuessedAtomChange to make sure I'm reading it right:

mgaudet: yeah, that looks right

tcampbell: thanks for gut check.

mrgiggles: pun

hello everyone!

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)?

JS::PersistentRooted leaves much cleaner code, so it seems that would be preferable pastebin.com/hz2J9wyz

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

elexis: I have a few notes on this in github.com/spidermonkey-embedders/s…blob/esr60/examples/tracing.cpp#L95

the biggest risk of persistentrooted is usually memory leaks

we had that problem too (one array with many JS objects vs one JS object with many items)

if no JS object owns a C++ object, there can't be a leak, right?

yep

(I think)

great!

the persistentrooted moves means the GC takes longer for the root-marking stage, rather than spreading over incremental slices

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.

mgaudet: lol.. I just noticed that

tcampbell: autoland being closed makes me wish there was an "oops" button on lando

elexis: my general advice would be that avoiding the persistentrooted forces you to be a bit more disciplined about GC tracing

right now by design no JS object owns a C++ object, so leaks arent of concern

so I wonder why we want this Tracer code to begin with if we could use a PersistentRooted

unless there is a performance reason

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

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

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

so the question is only: Is one JS::Heap object faster to keep track of for the GC than one JS::PersistentRooted object

(not considering allocation and deallocation)

marginally

elexis: I'm skimming the code. What the the name of an example GUI type?

oh, I see your pastebin

every GUI object type inherits the IGUIObject class, which keeps track of all of that

user opens a GUI page -> JS object is created -> 30 seconds pass -> user closes page

so the few microseconds upon opening / closing the page is not so relevant, more what happens during the 30 seconds

ah, so for that lifetime, the GCs will be marginally slower (but not really noticeable if you have ~100 objects)

I suppose there are < 500 objects tops on the ingame GUI page

PersistentRooted is definitely an improvement over JS_RemoveExtraGCRootsTracer

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

the code definitely looks nicer with PersistentRooted

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

and then trace the Heap that hang off each GUI node

if there isn't already a GUI tree relation, that might be ackward though

*awkward

there is a tree, but it's C++

that is fine

the trace methods are written in C++ in your code

eg. github.com/spidermonkey-embedders/s…blob/esr60/examples/tracing.cpp#L32

that is the recommended pattern that spidermonkey and gecko try to follow

mgaudet: just stack another patch for the rename FunctionBoxVector and I can stamp it

tcampbell: working on it -- took a couple extra minutes while I puzzled through what I wanted to do with the existing FunctionBoxVector type :P

ah, got it

hmm.. I didn't realize that existed

tcampbell: Neither did I

so I sunk it to its use, (nice for shortening the dependent declaration, but doesn't need huge scope like it has)

and have declared our function box type near AtomVector

nice

elexis: do you happen to know what version of spidermonkey the codebase is on?

38, but we're working on it :->

cool

(45 patch for review, 52 in some branch, and we wont stop there by the looks of things)

\o/

nice

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

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

in theory I thought JS::PersistentRooted would be faster, because the GC could always ignore it until its destroyed

persistentrooted means the gc has to check it every time during the beginning of a GC, and can't spread it over incremental

every edge still needs to be tested during the GC process

but the GC can split the work over mutliple slices that interleave with application work

I see

so the same amount of work, just not as well spread

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.

that would reduce the number of roots

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?

std::map<string, JSObject> could be changed to a JSObject containing the JSObjects however

that might be less complex than introducing a wrapper class

(did I get that right?)

if the trace method is needed, then why add the PersistentRooted pointer

only advantage would be spreading GC workload over multiple slices?

its currently already Heap, so I guess one doesnt need another persistentrooted container

sfink: bug 1570465 - bisecting now but I think this is yours

gkw: allegedly he's on PTO, the scoundrel

lol

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

generally you want to minimize the number of them and minimize how often they are come and go

those roots then point to an arbitrary number of objects in some sort of C++ graph that is your data structures

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

elexis: (If you haven't seen this, github.com/spidermonkey-embedders/s…/esr60/docs/GC%20Rooting%20Guide.md, some of the rooting reasoning is covered in there too)

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>|

oh right.. I ran into that :(.. I really hope the answer isn't that we need WrappedPtrOperations

Handle<PropertyDescriptor> works; not sure why it's special? searchfox.org/mozilla-central/sourc…/src/vm/ArgumentsObject.cpp#672,689

mgaudet: searchfox.org/mozilla-central/sourc…/js/public/PropertyDescriptor.h#278

=\

oh dear.

yeah.... bleh

I know if you have Rooted<UniquePtr<T>> you get sane behaviour

hrm. Heap allocation, or some extra .get() calls?

the eternal question

I.. don't know =\\

I"m leaning get()

but we'll see what you say when you see it in-situ

That will get() the job done

I suppose a third option is to have a const FunctionCreationData& local to the function; which seems even nicer

not the worst. We know the FunctionCreationData aint moving

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

yeah, still only on stack though

but the GC process still has to iterate over everything, it just can spread it better?

only the process of rooting every single item falls away?

correct

spidermonkey has to worry about keeping memory leaks quite a bit, so we avoid persistentrooted

sure

SM has to work with all applications in all circumstances

i.e. under the highest workload too

there is a minor thing that with one root you cross the library boundary less which is faster

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

confession: Reworking GC management in patch stack. how to root/handle tree is now tomorrow-Matthew's problem.

confession: Landed preq patches for allocation deferral

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

the IGUIObject class could be Rooted, sounds only a little bit like a monster

elexis: yeah, I think I'd agree with both of those

(I guess not the CGUI class, since that defines its own JSContext / JSRuntime to operate in)

(which is one instance per GUI page)

thank you a lot for taking the time and nerves to explain that tcampbell!

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 :->

:)

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

tcampbell: I wrote it!

tcampbell: it actually turns out to be useful for the Xray waiver issue I was having.

ah, cool

well, I don't need the finalize part of it.

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.

that's fine. I was just making sure I wasn't a bottleneck on reviews

confession: Made a start on threading jumps as part of the relaxation pass.