mozilla :: #ionmonkey

15 Mar 2017
14:09h4writerbbouvier: ping
14:19bbouvierh4writer: hi
14:20h4writerbbouvier: the windows 8 is getting stable ;). You'll now see the 64/32bit in the machine list
14:20bbouvierh4writer: thanks, i've seen it (and even asked about one thing in mp :))
14:20h4writerbbouvier: I also made sure the wasm one is working on chrome again ;)
14:21bbouvierha cool, cheers!
15:39bbouviernbp or jandem, ping
15:40bbouvieri was looking at the code of the ExecutableAllocator, and i see it tries to use small pools before creating a new independent pool; but these small pools seem to be released on gc sweep (via purge()), while the JitCode is still referencing them
15:40bbouvierwhat am i missing here?
15:47bbouvieris it that the JitCode is released as well during sweeping?
15:50nbpbbouvier: pong
15:50nbpbbouvier: They are ref-counted.
15:50nbpbbouvier: and the purge should filter out any page which has a non-zero refcount.
15:56nbpbbouvier: http://searchfox.org/mozilla-central/source/js/src/jit/Ion.cpp#802-838 This is the finalizers of JitCode objects which is called when sweeping.
15:58nbpbbouvier: The release function call below might free the page immediately: http://searchfox.org/mozilla-central/source/js/src/jit/ExecutableAllocator.cpp#48
16:00nbpbbouvier: Does that answer your questions?
16:00nbpbbouvier: JitCode are GC Things, so yes, they might be released during a GC, and finalized during sweeping.
16:04bbouviernbp: my question was specifically about the small pools, for which the release() function might be called two-fold: once in the ExecAlloc::purge() function, once in the JitCode finalizer
16:14nbpbbouvier: The ExecutableAllocator has a vector of ExecutablePool*, when we allocate a page, we increment the ref-count of the pool, as it is held by the ExecutableAllocator. When we gc, the ExecutableAllocator lose its reference to the ExecutablePool* elements, and decrement the ref-count of the ExecutablePool*. If there is still some JitCode alive on the pool, then the ref-count of it should keep the
16:14nbpExecutablePool* alive, otherwise the purge function will free it.
16:15nbpwhich makes me think that the python script to detect Jitted code is buggy.
16:20bbouviernbp: hum right, one thing i didn't see is that the JitCode finalizer seems to increase and decrease the ref count by 1, in all the cases
16:21bbouvierso finalizing JitCode may not free the pool, since the pool gets poisoned first
16:22bbouvierso the small pools never get destroyed, somehow? they start with refcount == 1, then if they're used refcount is incremented, then JitCodes have no effect on the refcount, but a purge() in gc sweeping will decrement the refcount
16:22bbouvierand at last, the refcount should be 1
16:22bbouvier(that is, the initial value)
16:23nbpbbouvier: it increase it in order to poison the content of the page, if this is a page with multiple JitCode inside.
16:24bbouviernbp: right, i assume it's decreased after poisoning
16:24nbpbbouvier: the vector which is going to do the poinsoning contains a reference to the ExecutablePool
16:24nbplth: You might want to remove this poisonning if you are going to use Perf for profilling.
16:25bbouviernbp: right, but it also decreases the refcount after poisoning: http://searchfox.org/mozilla-central/source/js/src/jit/ExecutableAllocator.cpp#378
16:25nbplth: otherwise you might have some hard time figuring out which "in" illegal-instruction is taking most of the time
16:25bbouviernbp: which mean that JitCode finalizers and thus JitCode in gneral has no net effect on the pool's refcount
16:26bbouviernbp: so for the small pools, where initial refcount is 1, and we inc by one in ExecutableAllocator::alloc(), i think the refcount ends up being always strictly non zero, thus this memory is never released
16:27nbpbbouvier: valgrind would have reported so.
16:27nbpbbouvier: I totally agree, following this ref-count logic is quite complex.
16:28bbouviernbp: ha you're right; probably the ExecutableAllocator ensures to release before destruction
16:28bbouvierso these small pools stay alive during the entire execution of spidermonkey, which kinda makes sense
16:29nbpbbouvier: the default 1 of the ref-count is made for the vector in which the pools are held by the ExecutableAllocator.
16:29nbpbbouvier: when we allocate a pool, we increment it, because it is going to be used by the JitCode.
16:30h4writerjandem: ping
16:30jandemh4writer: pong
16:30nbpbbouvier: when the JitCode is finalized, we increment it for poisoning later, and decrement it as it is no longer held by the JitCode.
16:30bbouviernbp: right, all of that i've followed
16:31nbpbbouvier: when we purge the ExecutableAllocator, we decrement the "default 1" of the ExecutablePool.
16:31bbouviernbp: i think the initial refcount of 2 for a small pool was incorrect, but i seem to get that we want these pools to be alive all the itme
16:31nbpbbouvier: when the poison is done, the ref-count of the old JitCode should be released, and the last JitCode alive should release the pages.
16:32nbpinitial refcount of 2? where?
16:33bbouviernbp: initialized to 1 in the ctor, then incremented for small pools in http://searchfox.org/mozilla-central/source/js/src/jit/ExecutableAllocator.cpp#149
16:33nbpbbouvier: because a JitCode will hold it.
16:34bbouviern
16:34bbouviernbp: well i think it doesn't call addRef() in all cases, but only for the small pools, in this function
16:35nbpbbouvier: yes, because large allocations are not listed in the ExecutableAllocator.
16:36nbpbbouvier: the ExecutableAllocator has no references to the large allocations.
16:36nbpbbouvier: http://searchfox.org/mozilla-central/source/js/src/jit/ExecutableAllocator.cpp#155,166-167
16:36bbouviernbp: okay, so large allocations aren't refcounted
16:37nbpbbouvier: they are but only by the JitCode.
16:39bbouviernbp: right, so the refcounting is in theory not much useful in this case
16:39nbpbbouvier: ok, so the "default 1" is not the ref-count of the executable allocator, but the ref-count of the first JitCode which is allocating the page
16:39bbouviernbp: okay, got it, thanks
16:45nbpbbouvier: The main problem is that all this is code are old C-ism
18:45nbph4writer: jandem: I tested the inlining backtracking, it seems to fail some MIRGraph assertion, I will check that later.
18:49lthnbp: ok
18:56h4writernbp: ok
18:57h4writernbp: hopefully it is something easy
16 Mar 2017
No messages
   
Last message: 104 days and 4 hours ago