github.com/moarvm/moarvm | IRC logs at colabti.org/irclogger/irclogger_logs/moarvm
Set by AlexDaniel on 12 June 2018.
07:58 cog joined 08:02 cog__ left
Geth MoarVM/hash-delete-corner-case: b8663f53ae | (Nicholas Clark)++ | 2 files
Fix a potential bug when all elements are individually deleted from a hash.

Two optimisations were coming together in an unforeseen way, which could cause an assertion failure on a debugging build.
There's a space optimisation for empty hashes which only allocates the control structure, and flags this with both `cur_items` and `max_items` ... (28 more lines)		 08:59
09:15 Altai-man joined
nwc10 and *this* time the flappy tests pass 09:36
Geth MoarVM: b8663f53ae | (Nicholas Clark)++ | 2 files
Fix a potential bug when all elements are individually deleted from a hash.

Two optimisations were coming together in an unforeseen way, which could cause an assertion failure on a debugging build.
There's a space optimisation for empty hashes which only allocates the control structure, and flags this with both `cur_items` and `max_items` ... (28 more lines)		 09:37
nwc10 oh wait, er, good *, #moarvm 09:38
and once more ENOCOFFEE
11:57 sena_kun joined 11:58 Altai-man left
timotimo oh hash yeah 12:06
15:56 Altai-man joined 15:58 sena_kun left 15:59 domidumont left, domidumont joined 17:14 zakharyas joined 18:23 domidumont left
MasterDuke hm. i'm getting segvs about cand->body.jitcode. for some ridiculous reason 0x18020100000001 isn't considered accessible. in 2020, can you believe it? 19:51
timotimo yeah, accessibility concerns have been on my mind lately 19:54
MasterDuke so i've created a new `spesh->body.spesh_candidates` my memcpying the relevant parts of the previous one. the candidate i'm getting rid of i've set its body.discarded to 1, i've freed the old spesh_candidates, decremented `spesh->body.num_spesh_candidates`, and called MVM_spesh_arg_guard_regenerate 19:55
what else do i need to do?
19:57 sena_kun joined 19:59 Altai-man left
timotimo phew 19:59
sorry, not sure
the 0x....001 thing is probably bogus, yeah?
MasterDuke yeah, the cand it's from is bogus also 20:00
(gdb) p *spesh_cand$1 = {common = {header = {sc_forward_u = {forwarder = 0xffffffff00000004, sc = {sc_idx = 4, idx = 4294967295}, st = 0xffffffff00000004}, owner = 1, flags1 = 1 '\001', flags2 = 2 '\002', size = 24}, st = 0x5555555b5470}, body = { cs = 0xffffffff00000004, type_tuple = 0x18020100000001, discarded = 56 '8', bytecode_size = 21845,
bytecode = 0xffffffff00000004 <error: Cannot access memory at address 0xffffffff00000004>, handlers = 0x18020100000001, spesh_slots = 0x5555555b5600, num_spesh_slots = 4, num_deopts = 4294967295, deopts = 0x18020100000001, deopt_count = 1432049352, deopt_named_used_bit_field = 18446744069414584324, deopt_pea = {materialize_info =
0x18020100000001, materialize_info_num = 93824992630672, materialize_info_alloc = 18446744069414584324, deopt_point = 0x18020100000001, deopt_point_num = 93824992630872, deopt_point_alloc = 18446744069414584324}, num_inlines = 1, inlines = 0x5555555b59e8, local_types = 0xffffffff00000005, lexical_types = 0x18020100000001, num_locals =
23216, num_lexicals = 21851, work_size = 21845, env_size = 5, num_handlers = 4294967295, jitcode = 0x18020100000001, deopt_usage_info = 0x5555555b5b78}}
20:38 leont joined
MasterDuke doesn't seem to happen under valgrind. is there some sort of locking/etc i should be doing? 20:57
timotimo hm, i think there might be a lock for spesh candidate handling actually? 20:58
nah, too performance-critical, must be a free-at-safepoint kind of thing 20:59
which i believe i actually saw in the code
jnthn Spesh candidate is a delicate dance that even has a memory barrier in there for good measure 21:01
It gets everything set up in the candidate list and only then installs it into the spesh arg guard data structure 21:02
MasterDuke jnthn: up for seeing the current diff?
jnthn There's no lock needed for installing new candidates because only one thread ever does it (and that'd be true for removing too), however everything you do is in a race with all the other threads which want to look up spesh candidates
Well, what you described above sounds problematic 21:03
Because there's a period when the spesh arg guard still references a candidate that is gone
MasterDuke yep, i believe so 21:04
jnthn I suspect the current design also draws on us only ever *adding* candidates 21:05
MasterDuke assuming removing is infrequent, add a lock only for removing?
jnthn That won't help. Remove themselves aren't racing. Rather, changes are (by design) racing with the threads that want to read candidates, and we can't really afford a lock there 21:06
MasterDuke ah, right 21:07
jnthn iiuc, currently we have 1) the arg guards, 2) an array of pointers to MVMSpeshCand objects, which are the ones you made GC-able? 21:08
Assuming that, there's two approaches I can think of 21:09
MasterDuke yep
timotimo we could have the array have holes in it
then at least old guard structures don't get outdated
jnthn 1) The boring one: add a level of indirection. Have a piece of memory that has pointers to current candidates and current guard tree. Only ever read that pointer once in any code path, and then use the consistent guard tree/candidate set pairing. 21:10
Then the usual update the point, and free at safepoint. 21:11
I think this is probably bulletproof.
MasterDuke bulletproof is good 21:12
jnthn 2) Try and retain something closer to the current approach to avoid the LoI, but don't renumber. Giving a "hole" that we can reuse later (after a safepoint, at least). I think the bookkeeping is scary.
So I'd do 1
It's an iota more memory churn, and there's the LoI, but I think that's a drop in the water and perhaps optimizable later respectively (e.g. finding a way to allocate one blob of memory for candidates and guard tree) 21:13
*drop in the ocean 21:14
MasterDuke so instead of MVMStaticFrameSpesh having a body.spesh_candidates, give it instead a body.spesh_candidates_and_their_associated_arg_guards_tree ? where spesh_candidates_and_their_associated_arg_guards_tree is something like `struct { MVMSpeshArgGuard *spesh_arg_guard; MVMSpeshCandidate **spesh_candidates; MVMuint32 num_spesh_candidates; }` 21:18
(extracted out from MVMStaticFrameSpeshBody)
jnthn Yes; to be clear, spesh_candidates_and_their_associated_arg_guards_tree must be a pointer to that struct, not just an inline struct
MasterDuke yeah 21:19
jnthn (that's the magic LoI that we derive the safety from)
MasterDuke every problem in computer science can be solved by adding a layer of indirection
jnthn I don't know where num_spesh_candidates best goes; you don't need it for resolution iirc
Except too many levels of indirection!
.oO( That's the one you solve with caching... )
 21:20
MasterDuke too bad layers of indirection don't wrap around eventually...
guess i'll start with leaving num_spesh_candidates where it is, fewer things to change 21:21
21:22 sena_kun left
jnthn Yeah, if you don't have to move it, don't. 21:22
21:43 zakharyas left
MasterDuke jnthn:btw, nice advent post 22:00
23:18 rypervenche left 23:41 rypervenche joined
jnthn MasterDuke: Thanks :) 23:42
23:48 leont left

Powered by Cro and the Raku® Programming Language.

Please report any issues / comments / feature requests as an issue on App::Raku::Log.

Thank you!