github.com/moarvm/moarvm | IRC logs at colabti.org/irclogger/irclogger_logs/moarvm Set by AlexDaniel on 12 June 2018. |
|||
00:05
pamplemousse joined
|
|||
timotimo | off to bed o/ | 00:22 | |
dogbert17 | good night | 00:23 | |
00:46
lucasb left
|
|||
discord6 | <timotimo> As I'm lying here trying to fall asleep, it occurs to me that perhaps the nursing thing is a write barrier, I.e. that MVM_ASSIGN should be used to say the type in the guard entry | 00:52 | |
<timotimo> The root would be whatever MVMObject it belongs to, which I'm not sure which one that is. Perhaps the frame? But then if it's not on the heap, what happens then... | 00:53 | ||
<timotimo> Or is return data an actual MVMObject? | 00:54 | ||
<timotimo> Anyway back to sleep attempts | |||
01:06
pamplemousse_ joined
01:09
pamplemousse left
01:13
pamplemousse_ left
01:15
pamplemousse_ joined
01:25
pamplemousse joined
01:28
pamplemousse_ left
01:50
pamplemousse left
01:55
pamplemousse joined
02:09
pamplemousse left
|
|||
samcv | ok. time for release | 05:13 | |
Geth | MoarVM/master: 17 commits pushed by (Ben Davies)++, (Stefan Seifert)++, (Jonathan Worthington)++, (Timo Paulssen)++, Altai-man++, (Samantha McVey)++ review: github.com/MoarVM/MoarVM/compare/2...744a47a2a5 |
05:25 | |
06:30
evalable6 left,
unicodable6 left,
greppable6 left
06:31
coverable6 joined,
greppable6 joined,
notable6 joined
06:32
shareable6 joined,
bloatable6 joined,
bisectable6 joined,
statisfiable6 joined
06:33
undersightable6 joined,
committable6 joined,
nativecallable6 joined
06:34
benchable6 joined,
evalable6 joined,
releasable6 joined,
unicodable6 joined,
reportable6 joined
06:35
squashable6 joined,
quotable6 joined
08:27
chloekek joined
|
|||
nine | A memset(other->nursery_fromspace, 0, other->nursery_fromspace_size); after GC yields some interesting results. Like a tc->cur_frame->caller->return_value pointing at a zeroed-out object tripping MVM_args_set_result_str | 09:12 | |
And suddenly I seem to find myself in the same area of code as dogbert17... | 09:18 | ||
I'd be surprised if we didn't have to MVMROOT name and plugin here: github.com/MoarVM/MoarVM/blob/mast...ugin.c#L20 | 09:32 | ||
09:46
brrt joined
|
|||
brrt | \o | 09:52 | |
nine | Oooh...the difference is: for me adding MVM_spesh_plugin_guard_list_mark(tc, srd->prev_plugin_guards, srd->prev_num_plugin_guards, worklist); to mark_plugin_sr_data actually fixes the issue! | ||
nwc10 | o/ | 09:54 | |
nine | At least in some cases... | 09:55 | |
brrt | hmmmm | ||
probably multiple issues then | |||
congratulations nine :-) | |||
nine | thanks :) | 09:56 | |
09:59
brrt left
|
|||
timotimo | nine: yeah, that does look like a place one would usually put an MVMROOT | 10:00 | |
probably difficult to hit as it'll only ever run once (per hll - so twice at most) | |||
10:01
brrt joined
10:10
brrt left
|
|||
nine | Ouch...it's not just the target frame's return_value that's NULL. The whole target frame is zeroed out | 10:28 | |
And this happens in: method result_kind() { $!result_kind } | 10:29 | ||
10:42
MasterDuke joined
|
|||
MasterDuke | anyone have an idea why `mp_lshd(<mp_int == 20>, 1)` would end up making the mp_int == 23058430092136939520? | 10:46 | |
dogbert17 | nine: very interesting | 10:52 | |
timotimo: managed got get a stack trace from the 'last GC' before failure | 10:53 | ||
s/got/to/ # need coffee | |||
dogbert17 fixes the coffee bug by decisive action | |||
timotimo: the stack trace from the last GC before the failure - gist.github.com/dogbert17/42f07a57...77651c0da2 | 10:57 | ||
11:01
sena_kun joined
|
|||
nine | God damn it C! Macros that look like function calls are devious. The segfault I traced was in autobox(tc, target, result, str_box_type, 1, set_str, target->return_value->o); which is not a function call but a macro. So the segfault didn't actually occur in this line but in the expanded macro. | 11:11 | |
And of course the macro gives an opportunity for GC and thus for target to move _before_ target->return_value->o is actually evaluated | |||
dogbert17 | nine++, you're finding bugs at breakneck speed | 11:13 | |
11:19
ggoebel_ joined
|
|||
Geth | MoarVM: 0a12e3c4e3 | (Stefan Seifert)++ | src/6model/reprs/ConcBlockingQueue.c Fix ConcBlockingQueue possibly unshifting an outdated item pointer An untimely GC may move to_add after we already wrote it to the newly created MVMConcBlockingQueueNode's value but before we add that node to the queue. Move the assignment down to match push's implementation which does it right. |
11:44 | |
MoarVM: f18bfadbe8 | (Stefan Seifert)++ | src/spesh/plugin.c Add missing MVMROOT in MVM_spesh_plugin_register name and plugin could be moved by the GC before we add them to spesh_plugins |
|||
MoarVM: 4ab1d425e1 | (Stefan Seifert)++ | src/core/args.c Fix autobox possibly writing to an outdated target Since autobox is a macro, the target argument is not evaluated at the place of the call but in the expanded code. That happens only after allocating, so the GC may have already moved the target frame. Avoid this by forgoing the local variable and accessing tc->cur_frame->caller again instead as that will have been updated. |
|||
nine | dogbert17: maybe these help with your issue as well? | ||
11:47
robertle joined
|
|||
dogbert17 | nine: will test immediately :) | 11:51 | |
nine | Though zeroing out the past fromspace helped with uncovering these issues, there is a snag with it: we actually don't free STables during GC, but do so in the next run. I.e. we actually keep using objects from the past fromspace. | 11:57 | |
dogbert17 | heh | 12:01 | |
I wonder if there are any GC related bugs left now | 12:06 | ||
nine | I'd bet there are ;) | 12:10 | |
I wonder if it'd still be worth it to memset everything but those STables | 12:16 | ||
dogbert17 | if it makes it easier to find bugs then some kind of option/flag to do that might be handy | 12:17 | |
nine | Yeah, I figured I would hide that memset behind MVM_GC_DEBUG anyway. So I may as well make it a little more involved | 12:18 | |
12:20
ggoebel_ left
|
|||
dogbert17 | unfortunately the bug in t/spec/S12-meta/primitives.t is still present | 12:29 | |
probably a good test for your zeroing logic | 12:30 | ||
12:40
ggoebel_ joined
12:58
brrt joined
|
|||
brrt | \o | 13:02 | |
dogbert17 | hi brrt | 13:16 | |
brrt | hi dogbert17 | 13:22 | |
13:22
brrt` joined
13:26
brrt left
|
|||
brrt` | nine++ fugbixing | 13:27 | |
13:32
brrt` left
13:34
lucasb joined
14:06
brrt` joined
|
|||
nine | I now get only a few failures with the zeroed-out fromspace (except for those STables). All of them seem to be related to hashes | 14:07 | |
14:13
brrt` left,
brrt` joined
|
|||
dogbert17 | interesting | 14:21 | |
15:05
brrt` left
|
|||
nine | BigInt operations are a real treasure trove of GC issues! | 15:16 | |
MVMP6bigintBody is inlined into MVMP6bigint, so even though it's not an MVMObject itself, it can get moved! While those bigint ops are MVMROOTing the bigint objects, they rarely take care to get the body's new address | 15:18 | ||
timotimo | oh damn | 15:20 | |
that's a good one | |||
and sounds rather dangerous | 15:21 | ||
nine | (Poisoning nursery_fromspace)++ | 15:22 | |
Geth | MoarVM: a001b7e4c7 | (Stefan Seifert)++ | src/math/bigintops.c Fix many memory corruption issues in bigint operations MVMP6bigintBody is inlined into MVMP6bigint, so even though it's not an MVMObject itself, it can get moved. While those bigint ops were MVMROOTing the bigint objects, they rarely took care of getting the body's new address. |
15:26 | |
timotimo | good eye | 15:28 | |
bummer we *just* released :) | |||
dogbert17 | hello timotimo | ||
I have gotten hold of the information you requested - gist.github.com/dogbert17/81c3aa3a...abf6034b78 | 15:30 | ||
timotimo | why does the backtrace look so strange :) | 15:31 | |
dogbert17 | I had to use some hacks | 15:32 | |
timotimo | i should have suggested to just break on MVM_gc_enter_from_allocator and use "commands", "bt", "c", "end" | 15:33 | |
and then "set pagination off" | |||
then it'd just have given a gdb backtrace for every gc run | 15:34 | ||
and you would just have had to look at the last one | |||
dogbert17 | my suspicions goes toward call_resolver - github.com/MoarVM/MoarVM/blob/mast...gin.c#L401 | ||
clever idea | |||
it calls setup_for_guard_recording can trigger a GC so what happens then to plugin_guards stuff? | 15:36 | ||
*which can trigger | 15:37 | ||
timotimo | i think the plugin_guards thing isn't a gc-moveable, it just contains a bunch of pointers that can move inside of it | 15:39 | |
and the tc roots should take care of that | |||
but i can still overlook something | |||
i don't know what MVM_frame_special_return does exactly, can it trigger gc? | |||
nine | IIRC it cannot | 15:41 | |
timotimo | in that case that function could be fine | ||
dogbert17 | at the start of call_resolver tc->plugin_guards are saved in some locals which are later in the function assigned to srd->prev_plugin_guards | 15:42 | |
I thought it had something to do with that but I'm probably mistaken | |||
timotimo | prev_plugin_guards gets rooted for the setup_for_guard_recording call | ||
nine | Oh boy.... with poisoning and a tiny nursery one doesn't have to look for bigints to find GC issues. They are there even in basic argument processing | ||
timotimo | i think that's the only place in there that can allocate | ||
nine: wow, that sounds promising | 15:43 | ||
dogbert17 | I believe so too | ||
nine | params is inlined in MVMFrame, so params can move. It's address is passed to _all_ param processing functions | ||
dogbert17 | shouldn't stuff like that crash all the time? | 15:44 | |
nine | Basically all of args.c is broken | ||
timotimo | i think because params is used read-only for most cases, it's usually fine? | 15:45 | |
nine | dogbert17: it does with the tiny nursery and a poisoned fromspace. Nothing passes anymore | ||
dogbert17 | quite the debug tool you have there :) | 15:46 | |
"Basically all of args.c is broken" - quote of the day | |||
timotimo | definitely still good to fix, in any case | 15:52 | |
15:54
domidumont joined
15:59
domidumont left
|
|||
nine | On a closer look, it's not as bad as I feared. Most functions in args.c only access ctx before GC can happen (if it does at all) | 16:01 | |
timotimo | well, that's good | ||
dogbert17 | timotimo: can you explain this, added check code to lines 402 and 413 - gist.github.com/dogbert17/753155d3...14c8b11657 | 16:13 | |
16:36
ggoebel_ left
|
|||
timotimo | can you check if the GC run there was a minor or major collection? | 16:37 | |
if it's a minor collection, it's possible that the data is only reachable via the gen2, so a missing gen2 root could be the issue | 16:41 | ||
if it's a minor collection, then there's a missing path via marking | |||
the valgrind gdbserver thingie has a "find pointers to this address" command | |||
that could be very interesting | |||
nine | Oh, capturenamedshash actually uses MVM_args_slurpy_named, too and here ctx is not just tc->cur_frame->params | 16:46 | |
timotimo | yeah it's for when you get an actual capture object with something like nqp::usecapture | 16:47 | |
i think? | 16:48 | ||
nine | Ah, so much passing :) | 16:53 | |
MasterDuke | nine: did you see my comment on github.com/MoarVM/MoarVM/commit/a001b7e4c7 ? | 17:00 | |
timotimo | bb is used a few lines lower, though | 17:05 | |
for the quick path "return a" | |||
nine | MasterDuke: just answered | 17:06 | |
MasterDuke | nine, timotimo: ah, but the `bb = get_bigint_body(tc, b);` needs to be repeated because 580 could have caused a GC and bb to move? | 17:08 | |
*b to move | |||
nine | yes | 17:10 | |
Geth | MoarVM: 64a6f69c92 | (Stefan Seifert)++ | src/core/exceptions.c Fix possible invalid memory access when throwing exceptions |
17:11 | |
17:17
domidumont joined
|
|||
Geth | MoarVM: e78080f67d | (Stefan Seifert)++ | src/strings/ops.c Fix possible access to fromspace in final_strand_match_with_repetition_count string_from_strand_at_index will allocate, so a and b may have moved even while the arguments to MVM_string_equal were evaluated. |
17:22 | |
nine | dogbert17: you worried about running out of GC issues? :D | 17:23 | |
sena_kun | nine++ # so many awesome fixes | 17:25 | |
Geth | MoarVM: e0bc6e2f1c | (Stefan Seifert)++ | 4 files Fix slurpy arg handling possibly accessing an outdated arg context The arg context is inlined into the frame, so even though its not an MVMCollectable itself it may move during GC. MVM_args_slurpy_named has 2 callers. For param_sn we want to access tc->cur_frame->params directly as that's gonna be up to date. The call capture in capturenamedshash does not inline the arg context, so we can use what we get passed without problems. |
17:35 | |
nine | There's quite a bit more to fix. Alas, dinner's almost ready and I promised a board game evening to my wife, so those fixes will have to wait. | 17:40 | |
dogbert17 | nine++, awesome fixes | ||
timotimo: it's a minor collection | 17:48 | ||
19:04
domidumont left
19:05
domidumont joined
19:25
domidumont left
|
|||
Kaiepi | how does MVM_UNLIKELY work? just ran into a case where code didn't work without it, but does with it | 20:00 | |
jnthn | That shouldn't happen... It's just a compiler hint about whether the branch is likely to be taken or not | 20:01 | |
Kaiepi | oh nvm, i was expecting it to invert whatever you passed to it | 20:06 | |
when are you supposed to use MVM_UNLIKELY? | 20:29 | ||
timotimo | when the code is hot and you're certain you know the outcome in the vast majority of cases | ||
usually type checks that end in an exception are a good place to put an UNLIKELY | 20:32 | ||
Kaiepi | wdym by the code is hot? | 20:36 | |
timotimo | is potentially executed lots of times per second | ||
it's not worth much to put an MVM_UNLIKELY in a spot that'll only be run once a second or so | 20:37 | ||
Kaiepi | oh, ok | ||
thanks | |||
Geth | MoarVM: 6730e51f92 | (Stefan Seifert)++ | src/strings/ops.c Fix possible access to fromspace in knuth_morris_pratt_string_index collapse_strands may allocate, so we need to keep Haystack updated |
20:47 | |
20:50
chloekek left
|
|||
Geth | MoarVM: 07f6305f45 | (Stefan Seifert)++ | src/strings/ops.c Fix possible access to fromspace in string_index_ignore_case We already correctly root Haystack but were assigning it to Hs_or_gic before that, so that one could have been outdated. Move Hs_or_gic's initialization to a place where Haystack is already stable. |
21:05 | |
MoarVM: ca9d44f2ea | (Stefan Seifert)++ | src/6model/reprconv.c Fix possible access to fromspace in MVM_repr_dimensions Allocating the result array can cause GC which may move obj, so move the allocation after the call to the repr function. We don't need it that early anyway. |
21:08 | ||
timotimo | anything in MVM_hll_map that could have been fixed recently? | ||
nine | not by me | 21:09 | |
timotimo | the moarvm heapanalyzer is able to b0rk with my new format stuff | 21:12 | |
huh, it's trying to read from a CArray via a nativeref after the carray has already been GC'd | 21:18 | ||
that's not good | |||
and valgrind itself apparently segfaulted, too | 21:22 | ||
21:25
squashable6 left
21:27
squashable6 joined
22:17
squashable6 left
|
|||
Geth | MoarVM: 8f425147a8 | (Samantha McVey)++ | 4 files Update Unicode to 12.1 |
22:18 | |
lizmat | samcv++ | 22:19 | |
22:21
squashable6 joined
22:22
squashable6 left,
squashable6 joined
22:28
squashable6 left
22:29
squashable6 joined
|
|||
MasterDuke | i have a rough translation of the Barrett stringification code into non-optimized C | 22:57 | |
it's still the recursive version with some string manipulation, but perf says the vast majority of the time is spent in libtommath multiplying numbers, so it doesn't seem like an optimized version would make a great difference | 22:59 | ||
to stringify 2**1_000_000, rakudo right now takes 57s, a pure perl6 version of the algorithm takes 3.3s, and the C version takes 2.7s | 23:01 | ||
timotimo | a million, that's pretty far out there | 23:03 | |
when do they break even? | |||
MasterDuke | pretty low. i can't measure too precisely, since there is some overhead in my timing of plain ~, but the C version seems just as fast even for tiny numbers, e.g., 2**6 | 23:08 | |
timotimo | brilliant | 23:10 | |
MasterDuke | timotimo: gist.github.com/MasterDuke17/95699...6a00d95ac2 | 23:12 | |
timotimo | i wonder if the code could sensibly re-use mp_int structs | 23:13 | |
i don't see any function calls to free mp_int structs | 23:14 | ||
MasterDuke | the code would need to be cleaned up and made to handle negative numbers for sure before it's used, but yeah, there are some optimizations to be had | 23:15 | |
timotimo | unfortunately, i'm not sure that's an optimization :) :) | 23:16 | |
MasterDuke | like you and the guy in the mailing list mentioned, a non-recursive version would make sense | ||
timotimo | i don't claim to have come up with that :) | 23:18 | |
jnthn is happy to see all the nice work going on :) | 23:27 | ||
timotimo | so right after the release we'll have to tell people "oh yeah go with latest master moarvm, the released one is full of crashes" :P | 23:28 | |
to be fair, the crashes mostly need a fair amount of artificial pressure to be applied to the GC | |||
AlexDaniel | jnthn: obligatory ping: github.com/rakudo/rakudo/issues/30...-515720444 :) | ||
although that's a pretty old bug :) | 23:29 | ||
timotimo: speaking of the releaseā¦ | |||
what's up with github.com/rakudo/rakudo/issues/3063 ? | |||
some people can reproduce it, some can't. I'm not even sure what the right steps are | 23:30 | ||
timotimo | what the heck? | ||
jnthn | AlexDaniel: huh, that's...a weird one, since it's just adding debug names, I think? | ||
timotimo | but turn on RAKUDO_MODULE_DEBUG or what it's called | ||
that could be interesting | |||
jnthn | AlexDaniel: I'm in theory on vacation next week, and then it's PerlCon the week after, so I'm not sure I'll get to investigate it terribly soon. | 23:31 | |
timotimo | oh | ||
the last comment in there has something interesting to say | |||
no clue what's up with that | 23:32 | ||
AlexDaniel | jnthn: yeah, could it be this? github.com/perl6/nqp/commit/a5232c...1f31b91057 | ||
timotimo | where do all the "omfg not a git repo" outputs come from | ||
AlexDaniel | that's the only commit that does somethingā¦ | ||
timotimo | is this a difference between from git clone and from release tarball? | ||
AlexDaniel | but I did confirm that that's the right bump: | ||
c: 34942c8597d^,34942c8597d use lib "sandbox/for-perl6-bug/lib"; my $aplugin = do { require ::("APlugin"); ::("APlugin"); }; say $aplugin.new.trycall(); | |||
committable6 | AlexDaniel, gist.github.com/3fdff0b768e86aa0ab...069ddd4baf | ||
AlexDaniel | timotimo: yes, and it's fixed for the point release | 23:33 | |
timotimo | ok cool | ||
jnthn | AlexDaniel: Feasibly, but it's odd nothing would have tripped over it before now if it were wrong...that's an extremely common code path. | ||
MasterDuke | heh. even the C version took 6m11s to stringify 2**10_000_000 (which, if anyone is curious, has 3,010,300 digits) | 23:35 | |
jnthn | And I've certainly got no idea what to do about it if it is to blame without having a much closer look | ||
timotimo | i assume the previous implementation we had would take upwards of an hour for that? ;) | ||
MasterDuke | i'm not going to punish my laptop by even trying | 23:36 | |
AlexDaniel | MasterDuke: what about trying the same in other languages? :) | 23:37 | |
well, not that large one, but simply benchmarking | 23:38 | ||
timotimo | does the algorithm in question parallelize at all? | 23:43 | |
it recurses in two parts, which sounds like "yes, in theory it could" | 23:44 | ||
but who knows what the overhead would be | |||
MasterDuke | for 2**300000, perl 5 takes 6.2s with 'use bigint', perl 6 takes 5.3s, python(2|3) takes 0.2s, ruby takes 0.07s | 23:46 | |
timotimo | that perl 6 figure is with what we have in master? | 23:47 | |
MasterDuke | yeah | ||
AlexDaniel | so what do they use in ruby? :) | 23:48 | |
MasterDuke | for 2**1000000, python(2|3) takes 1.5s, ruby takes 0.1s | ||
timotimo | should just Proc::Async ruby if it's installed :P | 23:49 | |
MasterDuke | so they're significantly faster than even my C version of the Barrett algorithm, which takes 2.7s | ||
timotimo | does perf point out what functions it's spending its time in? | ||
MasterDuke | i've just been counting chars, not printing them. maybe they have an optimization for that | 23:50 | |
timotimo | ah, possible | ||
MasterDuke | timotimo: | ||
timotimo | can try getting the first and last 10 chars or something | ||
that'd be unlikely to be optimized a lot | |||
MasterDuke | timotimo: s_mp_mul_digs | ||
82% | 23:51 | ||
timotimo | that's for ruby? | ||
MasterDuke | no, my c coe | ||
*code | |||
for ruby the top is __gmpn_mul_basecase, at 12% | 23:52 | ||
heh, for python3 it's 0x00000000001c2f30 at 12% and 0x00000000001c2f29 at 11% | 23:54 | ||
timotimo | hm, is that gnu gmp? | ||
MasterDuke | for ruby? i would assume so | ||
bugs.ruby-lang.org/issues/8796 | 23:55 | ||
timotimo | FWIW, if i'm looking correctly, getting only the first n chars of a biiiiig number should be pretty fast with barett? | ||
MasterDuke | i think so | 23:57 | |
timotimo | i wonder if that ever comes up in user code, like at all | ||
AlexDaniel | ah crap | 23:58 | |
I think rakudo fails to configure if you have ( in its path | 23:59 |