00:40 reportable6 joined 01:08 AlexDaniel joined 02:56 ilbot3 joined 03:23 greppable6 joined 03:24 val0rdkermit joined 05:15 unicodable6 joined 06:33 domidumont joined 06:41 domidumont joined 06:54 brrt joined 07:02 domidumont joined 08:15 brrt joined 08:18 robertle joined
Geth MoarVM: 2bd691551b | (Stefan Seifert)++ | src/6model/reprs/VMArray.c
Fix segfault on concurrent array access while resizing

nqp::elems(@a) == 1 && @a[0] eq 'a' may have caused a segfault when another thread did an nqp::push(@a, 'a'); at the same time. That's because set_size_internal first set body->elems (causing the nqp::elems(@a) == 1 test and internal checks in at_pos to pass) before actually installing the new slots array. Fix by updating body->elems last in set_size_internal.		 08:43
brrt nine++ 08:48
although, there is one eeny meeny tiny worry that I have, which is that this is now going to break more rarely than before, while it is just wron gcode to do so 08:49
the correct solution is probably to have an instrumentable mode for MoarVM to detect such cases though :-) 08:50
nine Well, jnthn says that even a misuse by concurrent code may not cause segfaults in MoarVM, so I fixed that. There's still a race condition in set_size_internal when we shift off empty slots from the beginning. And that one's not easy to fix. 08:54
We'd need to replace the memmove by creating a new slots array, memcpying over and thus replace the storage atomically. But then we'd still need to update body->start at the same time. 08:55
The only lock free way to achieve this would be to put most of VMArray's body including the slots array into a separate struct pointed to from the body. This way we could atomically replace the one pointer. But that gets very expensive quickly. 08:58
brrt either that or we get a lock 09:13
nine Which would tank our read performance 09:14
brrt 'don't do that then' :-P 09:15
lizmat brrt: that would have to be another repr, which would have its uses, but not in the general case
brrt yeah, I get it
then I'm not sure if we want a consistent thing here
well, i am 09:16
we want safe concurrent lock-free racing access
which my definition rules out a lot of possible implementations, including ones that would in 'normal cases' perform really well
09:33 robertle joined
jnthn I sometimes wonder if resizes are already rare/costly enough that an atomic op to install a newly resized array body would just be noise 10:12
yoleaux 07:46Z <nine> jnthn: do you concur with the reasoning? github.com/perl6/nqp/commit/68164ca223
jnthn In that, they aren't cheap, but realloc is even uncheaper 10:13
.tell nine yes 10:14
yoleaux jnthn: I'll pass your message to nine.
nine Ouch: ok 1 - Can create a new non-app-lifetime thread 10:17
yoleaux 10:14Z <jnthn> nine: yes
nine Failed 23/24 subtests
brrt depends, a small realloc isn't that expensive
nine Looks like that was a segfault in qp::threadrun($t); nqp::threadjoin($t);
MasterDuke wouldn't github.com/MoarVM/MoarVM/pull/689 (and then subsequent work to use the FSA for more VMArray things) help/prevent segv from concurrent access? 11:25
nine MasterDuke: I don't see how? 11:30
jnthn The FSA has a "free at safepoint" mechanism 11:31
Which in combination with an atomic compare and swap to install the newly resized storage would work 11:32
But
So that it's possibly to transactionally update the size and the storage of that size
The FSA's free at safepoint mechanism is indeed useful to ensure we avoid use-after-free (due to other threads still reading the memory) and ABA problems. 11:33
huh, one line I typed got lost?
The storage needs to also include the size too, and perhaps elems and start too 11:34
^^ was meant to go after the "But" :)
nwc10 "and a pony?" :-)
jnthn Well, a pointer-size pony would at least make the header bit before the storage a power of 2 :P 11:36
nine Ah, so that's essentially the "put the important bits into a struct and replace the pointer in the body" solution I described 11:38
jnthn Yeah, that + atomic op + FSA (or other means to avoid ABA and UAF) 11:40
I figure that the important info header can go on the start of the the memory region and the rest is storage 11:41
Though it throws allocation sizing off a little
Off page boundaries, I mean
nine I just managed to reproduce the segfault in nqp::threadrun($t); nqp::threadjoin($t); but again in a shell with disabled core dumps :/ 11:48
jnthn Also, MVMHash needs similar treatement, though that will mean moving away from "uthash" (not that what we actually have is much like the original uthash any more anyway) 11:49
Since it does an allocation per value
And we'd need just a single memory blob
Note that we probably cut down on a huge number of our allocations thanks to this
nwc10 I forget which hash implementation you had previously said looked interesting as a candidate to use/"steal" the design from 11:51
jnthn Me too :S
But there's a lot here at least
*a log, grr 11:55
nwc10 ++ # probablydance.com/2017/02/26/i-wro...hashtable/ 11:58
www.gnu.org/licenses/license-list.en.html#boost 12:00
probably hard to actually do something seen as stealing from it. This is useful. :-)
jnthn: are your fingers distracted by a lack of lunch? 12:01
jnthn Ah, indeed 12:03
That's to the license...though yeah, it's indeed already lunch time
jnthn bbiab 12:06
lizmat so, is nqp::splice(a,b,nqp::elems(a),0) smart enough to just replace a by b is a is empty ? 12:08
*if
13:41 FROGGS joined 13:42 AlexDaniel joined 14:40 FROGGS joined 16:25 brrt joined 16:26 brrt1 joined 16:53 domidumont joined 19:18 brrt joined 19:36 domidumont joined 19:49 brrt joined
lizmat is there a reason why we do have an nqp::splice, but no nqp::subbuf ? 19:56
samcv lizmat: subbuf would act like splice or does it just return a section of a buffer? but i believe that is something that should be added and we need 19:57
lizmat nqp::subbuf(buffer,pos,elems) basically 19:58
would give you an nqp::list with the elements of buffer from pos for elems elements
this could also simplify slicing like @a[10..20] significantly 19:59
if @a is fully reified :-)
it would also help in a hyperbatcher on reified arrays / lists :-) 20:01
timotimo i wanted a splice2 in the past 20:51
lizmat what would splice2 do ? 20:56
timotimo exactly what splice does, except takes one extra parameter for the start of the source (i think?) 20:58
lizmat that's already the 3rd parameter?
timotimo OK, then for the other one 21:05
samcv working on a Shift-JIS encoder right now. encoder seems to work. haven't integrated it in moarvm but going to implement the decoder and make sure it all works then move it into moarvm so i can test it more easily standalone 21:17
there are a bunch of different shift-jis encodings. so i decided to implement the one specified by W3C 21:18
lizmat samcv++
MasterDuke jnthn: are github.com/MoarVM/MoarVM/pull/689 and github.com/MoarVM/MoarVM/pull/773 (use the FSA for VMArray and MVMString respectively) still something you think we'll want at some point? 22:36
jnthn MasterDuke: Not totally sure on MVMString still. For VMArray yes, and it's a step on the way to some concurrency fixes discussed here earlier today 22:46
MasterDuke jnthn: cool. should i work on expanding the FSA use with VMArray (since that PR only uses it for deserialized and cloned arrays)? 22:55
jnthn MasterDuke: Could do, also maybe sync up with nine++ in case he's also thinking about working on the concurrency changes I suggested earlier 22:56
MasterDuke cool. re the existing PR, is it mergeable as is? or wait and see what nine's thinking? 22:59
jnthn I'd have to look again, and I've already more to do that I've hours left today, alas. 23:00
MasterDuke no worries, it's not keeping me up at nights 23:02
just glad to know it's still relevant 23:03
23:44 MasterDuke joined

Powered by Cro and the Raku® Programming Language.

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

Thank you!