Re: [Chicken-hackers] [PATCH] (hopefully) fix the massive random Salmonella breakage
Hi Peter,
This makes sense. I didn't know that was ever done, but after a search
of the rest of the runtime it doesn't *look* like this temporary stack
trick is used anywhere else. Nice find! I haven't been able to reproduce
a crash since applying these fixes on a handful of platforms.
Just two minor things: (1) it looks like one too many words is allocated
for the C_apply_values argvector, and (2) as a small performance tweak,
we can avoid inspecting all the pair headers a second time during
C_apply* by reusing the list's length when copying it into the argvector
(since we already know its length (and that it's a regular list) by that
point). See the attached patches. It's also a bit silly to shift, then
immediately unshift the C_u_i_length results, but that probably doesn't
matter much overall.
If you're happy with those two changes, I'll push the whole lot. Then we
can see a beautiful, error-free salmonella run tonight. :)
Evan
>From 705b28563f317d27ba09776b37f68233bac9b4a1 Mon Sep 17 00:00:00 2001
From: Evan Hanson
Date: Sun, 4 Oct 2015 20:09:19 +1300
Subject: [PATCH 1/2] Fix (harmless) off-by-one in C_apply_values
---
runtime.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/runtime.c b/runtime.c
index c981963..102fa42 100644
--- a/runtime.c
+++ b/runtime.c
@@ -6107,8 +6107,7 @@ void C_ccall C_apply_values(C_word c, C_word *av)
C_word
/* closure = av[ 0 ] */
k = av[ 1 ],
-lst,
-n;
+lst, len, n;
if(c != 3) C_bad_argc(c, 3);
@@ -6117,16 +6116,17 @@ void C_ccall C_apply_values(C_word c, C_word *av)
if(lst != C_SCHEME_END_OF_LIST && (C_immediatep(lst) || C_block_header(lst) != C_PAIR_TAG))
barf(C_BAD_ARGUMENT_TYPE_ERROR, "apply", lst);
- /* Check continuation wether it receives multiple values: */
+ /* Check whether continuation receives multiple values: */
if(C_block_item(k, 0) == (C_word)values_continuation) {
C_word *av2, *ptr;
-n = C_unfix(C_u_i_length(lst)) + 1;
+len = C_unfix(C_u_i_length(lst));
+n = len + 1;
if(!C_demand(n))
C_save_and_reclaim((void *)C_apply_values, c, av);
-av2 = C_alloc(n + 1);
+av2 = C_alloc(n);
av2[ 0 ] = k;
ptr = av2 + 1;
while(!C_immediatep(lst) && C_block_header(lst) == C_PAIR_TAG) {
--
2.5.3
>From 52412d6a14d779dc34012db1993f71376ea5b873 Mon Sep 17 00:00:00 2001
From: Evan Hanson
Date: Sun, 4 Oct 2015 20:25:56 +1300
Subject: [PATCH 2/2] Loop to known list length when copying args into av
during C_apply*
This avoids some unnecessary bit-twiddling, since we already know the
length of the list to copy.
---
runtime.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/runtime.c b/runtime.c
index 102fa42..10d70f7 100644
--- a/runtime.c
+++ b/runtime.c
@@ -5965,7 +5965,7 @@ void C_ccall C_apply(C_word c, C_word *av)
fn = av[ 2 ];
int av2_size, i, n = c - 3;
int non_list_args = n - 1;
- C_word lst, *ptr, *av2;
+ C_word lst, len, *ptr, *av2;
if(c < 4) C_bad_min_argc(c, 4);
@@ -5976,7 +5976,8 @@ void C_ccall C_apply(C_word c, C_word *av)
if(lst != C_SCHEME_END_OF_LIST && (C_immediatep(lst) || C_block_header(lst) != C_PAIR_TAG))
barf(C_BAD_ARGUMENT_TYPE_ERROR, "apply", lst);
- av2_size = 2 + non_list_args + C_unfix(C_u_i_length(lst));
+ len = C_unfix(C_u_i_length(lst));
+ av2_size = 2 + non_list_args + len;
if(!C_demand(av2_size))
C_save_and_reclaim((void *)C_apply, c, av);
@@ -5990,7 +5991,7 @@ void C_ccall C_apply(C_word c, C_word *av)
ptr += non_list_args;
}
- while(!C_immediatep(lst) && C_block_header(lst) == C_PAIR_TAG) {
+ while(len--) {
*(ptr++) = C_u_i_car(lst);
lst = C_u_i_cdr(lst);
}
@@ -6129,7 +6130,7 @@ void C_ccall C_apply_values(C_word c, C_word *av)
av2 = C_alloc(n);
av2[ 0 ] = k;
ptr = av2 + 1;
-while(!C_immediatep(lst) && C_block_header(lst) == C_PAIR_TAG) {
+while(len--) {
*(ptr++) = C_u_i_car(lst);
lst = C_u_i_cdr(lst);
}
--
2.5.3
___
Chicken-hackers mailing list
Chicken-hackers@nongnu.org
https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] (hopefully) fix the massive random Salmonella breakage
On Sun, Oct 04, 2015 at 10:32:01PM +1300, Evan Hanson wrote: > Hi Peter, > > This makes sense. I didn't know that was ever done, but after a search > of the rest of the runtime it doesn't *look* like this temporary stack > trick is used anywhere else. Nice find! I haven't been able to reproduce > a crash since applying these fixes on a handful of platforms. Cool. Have you been able to reproduce the crash (without patch) at all? > Just two minor things: (1) it looks like one too many words is allocated > for the C_apply_values argvector The argvector holds the continuation followed by each item in the argument list, which is why I added 1 to it. So I think my original patch is correct, and removing the +1 would in fact introduce an off-by-one error, unless I'm missing something here. > (2) as a small performance tweak, > we can avoid inspecting all the pair headers a second time during > C_apply* by reusing the list's length when copying it into the argvector > (since we already know its length (and that it's a regular list) by that > point). See the attached patches. Nice observation! I like this second patch. > It's also a bit silly to shift, then > immediately unshift the C_u_i_length results, but that probably doesn't > matter much overall. Yeah, I considered that too but I didn't want to inline that code again. We may refactor it into an "unboxed" version of the function that calculates the length as a native integer, and then redefine C_u_i_length in terms of that. This is probably something we can do across the entire runtime for various other functions, but that's a separate issue. > If you're happy with those two changes, I'll push the whole lot. Then we > can see a beautiful, error-free salmonella run tonight. :) That would be great. If you agree with my analysis about your first patch, please push my changes with your second patch only. Cheers, Peter signature.asc Description: Digital signature ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] (hopefully) fix the massive random Salmonella breakage
On 2015-10-04 14:55, Peter Bex wrote: > Cool. Have you been able to reproduce the crash (without patch) at all? Yeah, by simply running `make check` in a loop until it fails. It doesn't usually take more than 10 or 12 runs to hit the error (and only one or two when inside a VM). > > Just two minor things: (1) it looks like one too many words is allocated > > for the C_apply_values argvector > > The argvector holds the continuation followed by each item in the > argument list, which is why I added 1 to it. Yes, but that +1 is already done once before the C_demand (line 7304 after applying the first patch), then done again in the argument to C_alloc (line 7309). I think only the first one is necessary; that way we'll be C_alloc'ing the same amount that's C_demand'ed, and that's used for the eventual C_do_apply call, (+ (length lst) 1). Evan ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] (hopefully) fix the massive random Salmonella breakage
On Mon, Oct 05, 2015 at 08:06:29AM +1300, Evan Hanson wrote: > On 2015-10-04 14:55, Peter Bex wrote: > > Cool. Have you been able to reproduce the crash (without patch) at all? > > Yeah, by simply running `make check` in a loop until it fails. It > doesn't usually take more than 10 or 12 runs to hit the error (and only > one or two when inside a VM). Ah, that's interesting. Good to know! > > > Just two minor things: (1) it looks like one too many words is allocated > > > for the C_apply_values argvector > > > > The argvector holds the continuation followed by each item in the > > argument list, which is why I added 1 to it. > > Yes, but that +1 is already done once before the C_demand (line 7304 > after applying the first patch), then done again in the argument to > C_alloc (line 7309). I think only the first one is necessary; that way > we'll be C_alloc'ing the same amount that's C_demand'ed, and that's used > for the eventual C_do_apply call, (+ (length lst) 1). I overlooked that. Thanks for pointing it out! You're right of course, and I'm now convinced that your first patch is fine as well. Cheers, Peter signature.asc Description: Digital signature ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
Re: [Chicken-hackers] [PATCH] (hopefully) fix the massive random Salmonella breakage
Great, the whole set has been pushed. Now let's see what happens next. :) Evan ___ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
