Re: the new gc asserts in master
Ludovic Courtès escreveu: > Hi, > > Han-Wen Nienhuys <[EMAIL PROTECTED]> writes: > >> Ludovic Courtès escreveu: > >>> Note that what we agreed on was to provide ChangeLog-style comments in >>> the Git log entry, which this patch doesn't have. >> Can you explain me exactly what you want and why? > > I'm suggesting that we keep using ChangeLog-style entries, as per (info > "(standards) Change Logs"). > > This is in accordance with GCS and has proved to be a good auditing > tool. Auditing? how so? -- Han-Wen Nienhuys - [EMAIL PROTECTED] - http://www.xs4all.nl/~hanwen
Re: the new gc asserts in master
Hi, Han-Wen Nienhuys <[EMAIL PROTECTED]> writes: > Ludovic Courtès escreveu: >> Note that what we agreed on was to provide ChangeLog-style comments in >> the Git log entry, which this patch doesn't have. > > Can you explain me exactly what you want and why? I'm suggesting that we keep using ChangeLog-style entries, as per (info "(standards) Change Logs"). This is in accordance with GCS and has proved to be a good auditing tool. Ludo'.
Re: the new gc asserts in master
Ludovic Courtès escreveu: > Han-Wen Nienhuys <[EMAIL PROTECTED]> writes: > >> Pushed (without changelog entry). > > Note that what we agreed on was to provide ChangeLog-style comments in > the Git log entry, which this patch doesn't have. Can you explain me exactly what you want and why? I hope you're not suggesting that we add filenames, because git already tracks those for us. In many cases (see eg. git.git), people feel free to put much longer messages (entire emails) into the message. -- Han-Wen Nienhuys - [EMAIL PROTECTED] - http://www.xs4all.nl/~hanwen
Re: the new gc asserts in master
Han-Wen Nienhuys <[EMAIL PROTECTED]> writes: > Pushed (without changelog entry). Note that what we agreed on was to provide ChangeLog-style comments in the Git log entry, which this patch doesn't have. Thanks, Ludo'.
Re: the new gc asserts in master
Ludovic Courtès escreveu: >>> I'm still in favor of "git revert" since the log message makes it clear >>> which patch was reverted and why. "We" can then take our time and work >>> out a proper fix, and finally re-merge the patch plus its fix. >>> Furthermore, in the eventuality where none of us eventually finds a fix, >>> `master' is left in the previous state, which is better IMO. >> 'master' in its previous states grows the heap to 600M doing the 1000-fold >> version of srfi-18 test I posted. I think it's not a good solution. >> >> Commenting out the assert for x86-64 should yield better behavior. > > Alright, then please go ahead. Pushed (without changelog entry). -- Han-Wen Nienhuys - [EMAIL PROTECTED] - http://www.xs4all.nl/~hanwen
Re: the new gc asserts in master
Hello, Thanks for the nice summary! Andy Wingo <[EMAIL PROTECTED]> writes: > But what if it goes like this: > > S becomes collectable in theory > > mark phase: S is indeed marked as collectable > > C is returned from a callback: get_ptr() return S > > at some later time the card containing S is swept; S's free function > is run, and S is marked as a free cell > > at some later point maybe S gets reused for some other purpose > > however S was already alive in scheme, and we are using it as a smob! > > The point is: > > You cannot do C->Scheme mapping reliably in the presence of lazy > sweeping, because there is a time in which the object is marked as > sweepable but not swept, but the C->Scheme code has no way of knowing > this. > > (While talking with Ludovic we realized that his code has this problem.) The code I had in mind is GnuTLS [0], but it's a slightly more specific scenario and I now don't think the problem applies there. In GnuTLS, there are "session" objects; session objects can have a Scheme port attached to them, and the `session-record-port' procedure returns that port. What we want is: (eq? (session-record-port s) (session-record-port s)) IOW, there must be a mapping from the session to the port so that we don't create a new port each time `session-record-port' is called. Here's how it's achieved. Let `s' be the C session object, `S' the corresponding SMOB, and `P' the port (an `SCM'). We have the following object graph: SCM_STREAM(P) S < P | ^ |SCM_SMOB_DATA(S) | | | | _' | / gnutls_session_get_ptr(s) v/ s The mark procedure of `S' marks `P'. Thus, as long as `S' is reachable, `P' is reachable. In addition, as long as `P' is reachable, `S' is reachable. The important difference with the generalized scheme you described is that when `S' becomes unreferenced by Scheme code, there's no way `s' can suddenly reappear at the Scheme level because GnuTLS doesn't have any function that would return `s'. Thus, the race condition you described cannot happen. The key insight here is that `S' and `s' "aggregate" `P', i.e., the lifetime of `S' and `s' is always greater than or equal to that of `P'. I presume that this scheme is applicable to many (most?) object-oriented APIs. It's actually what lead to the inception of the `aggregated' typespec in G-Wrap [1]. Now, I haven't considered call-backs. But maybe a call-back can be seen as a procedure that's aggregated by some object; in turn, the procedure refer to other objects in its environment, such that the lifetime of the objects involved is similarly hierarchical. Sorry for the digression but I think it's important to know whether Guile's API intrinsically makes it hard to handle such common use cases. Thanks, Ludo'. [0] http://git.savannah.gnu.org/gitweb/?p=gnutls.git;a=blob;f=guile/src/core.c#l42 [1] http://www.nongnu.org/g-wrap/manual/Wrapping-a-C-Function.html
Re: the new gc asserts in master
Han-Wen Nienhuys escreveu: >>> The use of scm_gc_mark() outside of GC is fundamentally broken, since it >>> creates race conditions in the presence of threads. >> I was not aware that this was the case. >> >> My impression was that the mark phase is global; it requires all threads >> that were in guile mode to go dormant, and those that were not in guile >> mode cannot enter guile mode until the mark is complete. > > Yes, the mark phase is global, but the thread locking is done in > scm_i_gc; once the marking starts, there is only one thread. Since > scm_gc_mark is called from the smob mark functions, it does not force > other threads to go dormant. It could, but I suspect the lock would > be a contention point. It would be very cool to have thread safe marking for a different reason: marking it is the expensive step in GC, so if we can do that in N threads concurrently (on a SMP machine) we have can speed it up by almost a factor N. To do it properly, you could do the bitvector marking with a compare & swap instruction. -- Han-Wen Nienhuys - [EMAIL PROTECTED] - http://www.xs4all.nl/~hanwen
Re: the new gc asserts in master
Han-Wen Nienhuys <[EMAIL PROTECTED]> writes: > Ludovic Courtès escreveu: >> I'm still in favor of "git revert" since the log message makes it clear >> which patch was reverted and why. "We" can then take our time and work >> out a proper fix, and finally re-merge the patch plus its fix. >> Furthermore, in the eventuality where none of us eventually finds a fix, >> `master' is left in the previous state, which is better IMO. > > 'master' in its previous states grows the heap to 600M doing the 1000-fold > version of srfi-18 test I posted. I think it's not a good solution. > > Commenting out the assert for x86-64 should yield better behavior. Alright, then please go ahead. Thanks, Ludo'.
Re: the new gc asserts in master
Andy Wingo escreveu: > Hi again! > > On Wed 27 Aug 2008 12:00, Andy Wingo <[EMAIL PROTECTED]> writes: > >> On Wed 27 Aug 2008 07:00, Han-Wen Nienhuys <[EMAIL PROTECTED]> writes: >> >> http://thread.gmane.org/gmane.lisp.guile.user/6372 >>> I think reference counting is the correct solution for this, as far as >>> I understand the problem from the quoted message. >> I don't think so; the use case is that (1) we don't want to prevent the >> C object from being freed, so we don't want to hold a reference on the C >> object; but (2) we do want to know when it is freed, so we can release >> our cache; but (3) we want to get the scheme object back if the object >> has not in fact been swept. As far as I understand, you are splitting the decision whether an object is live (reachable) between C and SCM - it's reachable when there is an SCM reference, but it is also reachable when there is no SCM reference anymore, but it is still reachable by looking at a C table and returning the SCM reference inside the C struct. You can never get a consistent model if you try to split it up like that. Either the memory management is entirely in SCM (using (weak) hash tables in SCM and GC which decides whether an object is reachable) or the memory management is entirely in C (eg. using ref counting). If you try to mix both, you'll get a frankenstein with cloudy semantics and edgy corner cases. > I don't think this is exactly right. I was discussing this on IRC with > Ludovic and he made me come out with a better characterization of the > problem. > > Consider a C object, `C'. We wrap it in scheme with a SCM object, `S'. S > has a reference on C, using reference counting. C has some kind of API > to associate S with it: set_ptr() and get_ptr(). Cool. > > So what happens if we get C back from a callback at some time in the > future? Well we call get_ptr(C) and return that if it's non-null. > Otherwise we make a new smob and call set_ptr (C, S) and then return S. > > So what happens if the scheme object becomes collectable? Well S has a > free function which will unref the C object and set_ptr (C, NULL). This > is also OK. > > But what if it goes like this: > > S becomes collectable in theory > > mark phase: S is indeed marked as collectable > > C is returned from a callback: get_ptr() return S > > at some later time the card containing S is swept; S's free function > is run, and S is marked as a free cell Here you are dividing the responsibity of liveness between SCM and C. SCM decides the object is dead, but you hold on to it in in C, and then insert the dead reference back into SCM, leading to crappage. I think it is wrong to equate (destructor has run) with (object is reachable). One of the important points of GC (RAII will say it's a detractor) vs. C++ smart pointer is that going out of scope and running the destructor to go with that can happen at different points in time. Between these points, the objects are in a sort of limbo: you can't do return them back to the 'live' part of the program, but the destructror hasn't run yet. -- Han-Wen Nienhuys - [EMAIL PROTECTED] - http://www.xs4all.nl/~hanwen
Re: the new gc asserts in master
Ludovic Courtès escreveu: > Sorry if I missed something but my understanding was that you were > referring to a GC fix, which it isn't. Re-reading the thread, it seems > I indeed missed the point, and I apologize. I hope you do realize that every time you miss the point and send out a reply you have me trembling with anger, and contemplating whether I should stop contributing to GUILE right away or after it's patched up enough for LilyPond? After some years of doing lilypond, I have become wise enough to delete the expletives before I send an email, but please think and reread before you send out something. -- Han-Wen Nienhuys - [EMAIL PROTECTED] - http://www.xs4all.nl/~hanwen
Re: the new gc asserts in master
Ludovic Courtès escreveu: >> +#if (SCM_DEBUG_CELL_ACCESSES == 0 && SCM_SIZEOF_UNSIGNED_LONG == 4) > > x86-64 is not the only arch with 4-byte long long integers. I'm not pretending it is the end-all fix. - we (I) need to understand what is happening and make the numbers match up exactly. This is just a kludge. > I'm still in favor of "git revert" since the log message makes it clear > which patch was reverted and why. "We" can then take our time and work > out a proper fix, and finally re-merge the patch plus its fix. > Furthermore, in the eventuality where none of us eventually finds a fix, > `master' is left in the previous state, which is better IMO. 'master' in its previous states grows the heap to 600M doing the 1000-fold version of srfi-18 test I posted. I think it's not a good solution. Commenting out the assert for x86-64 should yield better behavior. -- Han-Wen Nienhuys - [EMAIL PROTECTED] - http://www.xs4all.nl/~hanwen
Re: the new gc asserts in master
Andy Wingo escreveu: >> I think reference counting is the correct solution for this, as far as >> I understand the problem from the quoted message. > > I don't think so; the use case is that (1) we don't want to prevent the > C object from being freed, so we don't want to hold a reference on the C > object; but (2) we do want to know when it is freed, so we can release > our cache; but (3) we want to get the scheme object back if the object > has not in fact been swept. I don't understand how reference counting would prevent you from doing this. Reference counting only is broken if you have (lots of) cyclical references. Do you have those? With ref-counting you would loose the unique C object <-> SCM object mapping, since you could create another reference (another SCM object) for a given C object if a SCM reference was lost, but a new one created in the same GC cycle. From what I've read, this should not be a problem. Are you relying on the SCM representation of the C object being unique? If not, there should not be a problem. > But the laziness of the sweeper prevents us from knowing whether the > cache that we have is in fact accessible, because there is a time > between the mark phase (in which the object might become sweepable) and > when the smob's free function is called in the sweep phase (which would > invalidate the cache). >> The use of scm_gc_mark() outside of GC is fundamentally broken, since it >> creates race conditions in the presence of threads. > > I was not aware that this was the case. > > My impression was that the mark phase is global; it requires all threads > that were in guile mode to go dormant, and those that were not in guile > mode cannot enter guile mode until the mark is complete. Yes, the mark phase is global, but the thread locking is done in scm_i_gc; once the marking starts, there is only one thread. Since scm_gc_mark is called from the smob mark functions, it does not force other threads to go dormant. It could, but I suspect the lock would be a contention point. If you call scm_gc_mark() on your own schedule, you're venturing outside the established model. From what I understand from your description, you detect that an object is live outside of the mark phase, which is what triggers the assertion, and which does not force thread correctness. > So if I have a thread in guile mode, it is not in the mark phase, hence > no race. Also, it would not be sweeping; I can check the cache and > retrieve and mark the object without the thread of interest doing a > sweep(). But perhaps some other thread would sweep that card, in which > case I guess I can see where the problem would come in. An alternative is to have a GC hook, like struct does. It does tie you to the GC implementation. I think something that works regardless of the GC details is better for a user of GUILE. > It's very irksome that I missed this bit in the documentation. > > So, my proposal to fix this is to expose the sweep mutex as part of the > API somehow, perhaps as e.g. > >void* scm_with_sweep_mutex (void* (*with_mutex_func)(void*), void*); > > or so. How do you feel about this? I know it constrains your GC > implementation, but threads + lazy sweeping + integrating with C > libraries = exposing some minimal amount of low-level details. I think it is best to expose as little of the GC implementation as possible. The fact that there is a mutex already gives away implementation details. I'm a bit miffed that the current interface already gives away that we won't rewrite (ie. compact) the current heap. Let's not paint ourselves in more corners. -- Han-Wen Nienhuys - [EMAIL PROTECTED] - http://www.xs4all.nl/~hanwen
Re: the new gc asserts in master
Hi again! On Wed 27 Aug 2008 12:00, Andy Wingo <[EMAIL PROTECTED]> writes: > On Wed 27 Aug 2008 07:00, Han-Wen Nienhuys <[EMAIL PROTECTED]> writes: > > http://thread.gmane.org/gmane.lisp.guile.user/6372 >> >> I think reference counting is the correct solution for this, as far as >> I understand the problem from the quoted message. > > I don't think so; the use case is that (1) we don't want to prevent the > C object from being freed, so we don't want to hold a reference on the C > object; but (2) we do want to know when it is freed, so we can release > our cache; but (3) we want to get the scheme object back if the object > has not in fact been swept. I don't think this is exactly right. I was discussing this on IRC with Ludovic and he made me come out with a better characterization of the problem. Consider a C object, `C'. We wrap it in scheme with a SCM object, `S'. S has a reference on C, using reference counting. C has some kind of API to associate S with it: set_ptr() and get_ptr(). Cool. So what happens if we get C back from a callback at some time in the future? Well we call get_ptr(C) and return that if it's non-null. Otherwise we make a new smob and call set_ptr (C, S) and then return S. So what happens if the scheme object becomes collectable? Well S has a free function which will unref the C object and set_ptr (C, NULL). This is also OK. But what if it goes like this: S becomes collectable in theory mark phase: S is indeed marked as collectable C is returned from a callback: get_ptr() return S at some later time the card containing S is swept; S's free function is run, and S is marked as a free cell at some later point maybe S gets reused for some other purpose however S was already alive in scheme, and we are using it as a smob! The point is: You cannot do C->Scheme mapping reliably in the presence of lazy sweeping, because there is a time in which the object is marked as sweepable but not swept, but the C->Scheme code has no way of knowing this. (While talking with Ludovic we realized that his code has this problem.) My solution was to re-mark when you do get_ptr(), but I see now where the problem with that is. >void* scm_with_sweep_mutex (void* (*with_mutex_func)(void*), void*); I think this is not so elegant, but it is correct. Andy -- http://wingolog.org/
Re: the new gc asserts in master
Hi, On Wed 27 Aug 2008 07:00, Han-Wen Nienhuys <[EMAIL PROTECTED]> writes: http://thread.gmane.org/gmane.lisp.guile.user/6372 > > I think reference counting is the correct solution for this, as far as > I understand the problem from the quoted message. I don't think so; the use case is that (1) we don't want to prevent the C object from being freed, so we don't want to hold a reference on the C object; but (2) we do want to know when it is freed, so we can release our cache; but (3) we want to get the scheme object back if the object has not in fact been swept. But the laziness of the sweeper prevents us from knowing whether the cache that we have is in fact accessible, because there is a time between the mark phase (in which the object might become sweepable) and when the smob's free function is called in the sweep phase (which would invalidate the cache). > The use of scm_gc_mark() outside of GC is fundamentally broken, since it > creates race conditions in the presence of threads. I was not aware that this was the case. My impression was that the mark phase is global; it requires all threads that were in guile mode to go dormant, and those that were not in guile mode cannot enter guile mode until the mark is complete. So if I have a thread in guile mode, it is not in the mark phase, hence no race. Also, it would not be sweeping; I can check the cache and retrieve and mark the object without the thread of interest doing a sweep(). But perhaps some other thread would sweep that card, in which case I guess I can see where the problem would come in. It's very irksome that I missed this bit in the documentation. So, my proposal to fix this is to expose the sweep mutex as part of the API somehow, perhaps as e.g. void* scm_with_sweep_mutex (void* (*with_mutex_func)(void*), void*); or so. How do you feel about this? I know it constrains your GC implementation, but threads + lazy sweeping + integrating with C libraries = exposing some minimal amount of low-level details. Regards, Andy -- http://wingolog.org/
Re: the new gc asserts in master
Hello Han-Wen,
Han-Wen Nienhuys <[EMAIL PROTECTED]> writes:
> Over the last weeks I have seen little discussion on your patches, eg.
Please, refrain from resorting to personal attacks.
> commit 582a4997abc8b34ac6caf374fda8ea3ac65bd571
> Author: Ludovic Courtès <[EMAIL PROTECTED]>
> Date: Mon Aug 25 11:20:02 2008 +0200
>
> Use $(GCC_CFLAGS) for `-Werror' et al. so that it's not used to compile
> Gnulib code.
>
> commit c95514b3b41c8e335ada863f8abb99cc4af9abe1
> Author: Ludovic Courtès <[EMAIL PROTECTED]>
> Date: Thu Aug 14 00:15:03 2008 +0200
>
> Remove the now useless `qthreads.m4'.
These patches are trivial IMO and contain a descriptive git log and
ChangeLog entry. Compare this with the ten-or-so patches that you
committed, which included a far-from-trivial rework of the GC.
>From my observations, Neil, Kevin and I have always worked this way,
although this was never formally specified. Perhaps Neil can comment?
> The commit message says,
>
> Set SRCPROP{PLIST,COPY} through a macro, so SCM_DEBUG_CELL_ACCESSES
> compiles.
>
> how much clearer do you want this message to be?
Sorry if I missed something but my understanding was that you were
referring to a GC fix, which it isn't. Re-reading the thread, it seems
I indeed missed the point, and I apologize. For my defense, I'd say
that I didn't expect a "GC cleanup" to touch all that.
even the lazy smob case I wrote about here:
http://thread.gmane.org/gmane.lisp.guile.user/6372
>>> I would classify the use of mark bits outside of the mark phase as outside
>>> of the defined API. If you want to have weak pointer semantics, use
>>> a weak hashtable, or implement reference counting on the C side.
>>
>> That's a reasonable argument, but it's something we should not change
>> without discussing it first. For instance, it may be important to study
>> why Guile-GNOME had to resort to this, and how it could avoid it,
>> instead of just gratuitously breaking it.
>
> I'm not suggesting to change without discussing; this message rather
> is the start of the discussion.
Good. However, my understanding of Andy's message was that the "GC
cleanups" already changed that, making Guile-GNOME's recipe fail.
> From ccd010e15ec0ddf285b75911739e85866d2d865c Mon Sep 17 00:00:00 2001
> From: Han-Wen Nienhuys <[EMAIL PROTECTED]>
> Date: Wed, 27 Aug 2008 10:48:06 -0300
> Subject: [PATCH] Kludge around x86-64 GC runtime checks.
>
> 2008-08-27 Han-Wen Nienhuys <[EMAIL PROTECTED]>
>
> * gc.c (scm_i_gc): Don't sanity check numbers on x64, while we
> investigate a real fix.
>
> ---
> libguile/gc.c |4 ++--
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/libguile/gc.c b/libguile/gc.c
> index 8e8769c..a0b3080 100644
> --- a/libguile/gc.c
> +++ b/libguile/gc.c
> @@ -597,9 +597,9 @@ scm_i_gc (const char *what)
>scm_i_sweep_all_segments ("GC", &scm_i_gc_sweep_stats);
>scm_check_deprecated_memory_return ();
>
> +#if (SCM_DEBUG_CELL_ACCESSES == 0 && SCM_SIZEOF_UNSIGNED_LONG == 4)
x86-64 is not the only arch with 4-byte long long integers.
I'm still in favor of "git revert" since the log message makes it clear
which patch was reverted and why. "We" can then take our time and work
out a proper fix, and finally re-merge the patch plus its fix.
Furthermore, in the eventuality where none of us eventually finds a fix,
`master' is left in the previous state, which is better IMO.
Would you like that solution?
Thanks,
Ludo'.
Re: the new gc asserts in master
Ludovic Courtès escreveu:
> Han-Wen Nienhuys <[EMAIL PROTECTED]> writes:
>
>> I am pushing a fix for this to master.
>
> Will you care to post and discuss your patches before pushing them?
Over the last weeks I have seen little discussion on your patches, eg.
commit 582a4997abc8b34ac6caf374fda8ea3ac65bd571
Author: Ludovic Courtès <[EMAIL PROTECTED]>
Date: Mon Aug 25 11:20:02 2008 +0200
Use $(GCC_CFLAGS) for `-Werror' et al. so that it's not used to compile
Gnulib code.
commit c95514b3b41c8e335ada863f8abb99cc4af9abe1
Author: Ludovic Courtès <[EMAIL PROTECTED]>
Date: Thu Aug 14 00:15:03 2008 +0200
Remove the now useless `qthreads.m4'.
Were pushed without review. There was a post on
commit 450be18dfffd496ef14e1c921953e6f179727ab4
Author: Ludovic Courtès <[EMAIL PROTECTED]>
Date: Thu Jul 17 00:17:56 2008 +0200
Handle lack of `struct dirent64' and `readdir64_r ()' on HP-UX 11.11.
but it was after the fact
Hi,
FYI, I committed the attached patch, which handles the lack of `struct
dirent64' and `readdir64_r ()' on HP-UX 11.11 (and possibly other
versions).
I'm assuming here that you -in good community spirit- don't consider
yourself to be above your own rules.
In other words: you can't do this. If you want people to discuss
before pushing you should set the good example.
> This is all the more important that the patches don't seem to have any
> relation with the problem at hand:
>
> f85ea2a85fcdd051f432964806f044c0301d0945 Merge branch 'master' of
> git://git.sv.gnu.org/guile into nits
> 487b9dec2ea6b88ddbc6fbd17f445ddb197aebc5 Only sanity check numbers if
> SCM_DEBUG_CELL_ACCESSES is unset.
> 80237dcc7783b4d94ecf1d987deb9306d61735a0 Set SRCPROP{PLIST,COPY} through a
> macro, so SCM_DEBUG_CELL_ACCESSES compiles.
This in reference to GUILE not compiling with SCM_DEBUG_CELL_ACCESSES
The commit message says,
Set SRCPROP{PLIST,COPY} through a macro, so SCM_DEBUG_CELL_ACCESSES
compiles.
how much clearer do you want this message to be? It is fixing a
compilation problem for a certain preprocessor define. It doesn't
pretend to fix x86-64. You were complaining before that my changes
were too large and should have been split up. This patch is split
up.
Can you make up your mind?
> Do you think you can come up with a fix within the next few days?
In the spirit of your undocumented development and
community standards, I am including below a patch for this
problem. Let's discuss this complex change first to decide whether it
is worthy for inclusion in the oh-so-active GUILE repository.
> Otherwise, I'm inclined to revert the offending commits in `master' and
> wait for a signal from you (i.e., a patch or merge request posted to the
> mailing list, *not* a commit on `master'). It would make it easier for
> us to play with `master' in the meantime.
> Besides, avoid pushing from an non-up-to-date repo: this yields to
> automatic merges like the one above, which is annoying as it makes
> history harder to follow. Better pull first, then merge your changes,
> then push.
Can you document your requirements upfront instead complaining after the fact?
[EMAIL PROTECTED] guile]$ grep -i merge HACKING
[EMAIL PROTECTED] guile]$
[EMAIL PROTECTED] guile]$ grep -i pull HACKING
[EMAIL PROTECTED] guile]$
>>> even the lazy smob case I wrote about here:
>>>
>>> http://thread.gmane.org/gmane.lisp.guile.user/6372
>> I would classify the use of mark bits outside of the mark phase as outside
>> of the defined API. If you want to have weak pointer semantics, use
>> a weak hashtable, or implement reference counting on the C side.
>
> That's a reasonable argument, but it's something we should not change
> without discussing it first. For instance, it may be important to study
> why Guile-GNOME had to resort to this, and how it could avoid it,
> instead of just gratuitously breaking it.
I'm not suggesting to change without discussing; this message rather
is the start of the discussion. I think reference counting is the
correct solution for this, as far as I understand the problem from the
quoted message.
The use of scm_gc_mark() outside of GC is fundamentally broken, since it
creates race conditions in the presence of threads.
--
Han-Wen Nienhuys - [EMAIL PROTECTED] - http://www.xs4all.nl/~hanwen
>From ccd010e15ec0ddf285b75911739e85866d2d865c Mon Sep 17 00:00:00 2001
From: Han-Wen Nienhuys <[EMAIL PROTECTED]>
Date: Wed, 27 Aug 2008 10:48:06 -0300
Subject: [PATCH] Kludge around x86-64 GC runtime checks.
2008-08-27 Han-Wen Nienhuys <[EMAIL PROTECTED]>
* gc.c (scm_i_gc): Don't sanity check numbers on x64, while we
investigate a real fix.
---
libguile/gc.c |4 ++--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/libguile/gc.c b/libguile/gc.c
index 8e8769c..a0b3080 100644
--- a/libguile/gc.c
+++ b/libguile/gc.c
@@ -597,9 +597,9 @@ scm_i_gc (const char *what)
scm_i_sweep_all_segments ("GC",
Re: the new gc asserts in master
Hi,
Han-Wen Nienhuys <[EMAIL PROTECTED]> writes:
> I am pushing a fix for this to master.
Will you care to post and discuss your patches before pushing them?
This is all the more important that the patches don't seem to have any
relation with the problem at hand:
f85ea2a85fcdd051f432964806f044c0301d0945 Merge branch 'master' of
git://git.sv.gnu.org/guile into nits
487b9dec2ea6b88ddbc6fbd17f445ddb197aebc5 Only sanity check numbers if
SCM_DEBUG_CELL_ACCESSES is unset.
80237dcc7783b4d94ecf1d987deb9306d61735a0 Set SRCPROP{PLIST,COPY} through a
macro, so SCM_DEBUG_CELL_ACCESSES compiles.
Can you please describe them and add ChangeLog entries (yes, we still
use that)?
In addition, they don't fix anything on x86-64:
$ ./pre-inst-guile
lt-guile: gc.c:610: scm_i_gc: Assertion `scm_i_gc_sweep_stats.collected +
scm_cells_allocated == scm_i_gc_sweep_stats.swept' failed.
Aborted (core dumped)
Do you think you can come up with a fix within the next few days?
Otherwise, I'm inclined to revert the offending commits in `master' and
wait for a signal from you (i.e., a patch or merge request posted to the
mailing list, *not* a commit on `master'). It would make it easier for
us to play with `master' in the meantime.
Besides, avoid pushing from an non-up-to-date repo: this yields to
automatic merges like the one above, which is annoying as it makes
history harder to follow. Better pull first, then merge your changes,
then push.
>> even the lazy smob case I wrote about here:
>>
>> http://thread.gmane.org/gmane.lisp.guile.user/6372
>
> I would classify the use of mark bits outside of the mark phase as outside
> of the defined API. If you want to have weak pointer semantics, use
> a weak hashtable, or implement reference counting on the C side.
That's a reasonable argument, but it's something we should not change
without discussing it first. For instance, it may be important to study
why Guile-GNOME had to resort to this, and how it could avoid it,
instead of just gratuitously breaking it.
Thanks,
Ludo'.
Re: the new gc asserts in master
Han-Wen Nienhuys escreveu: >> even the lazy smob case I wrote about here: >> >> http://thread.gmane.org/gmane.lisp.guile.user/6372 > > I would classify the use of mark bits outside of the mark phase as outside > of the defined API. If you want to have weak pointer semantics, use > a weak hashtable, or implement reference counting on the C side. > > I am actuallly inclined to add add abort() for anyone who calls scm_gc_mark() > outside the marking phase. Also, you're creating a race condition: the mark bits are not protected by a lock, so you will be screwed in still more interesting ways if more threads or types would start doing this. -- Han-Wen Nienhuys - [EMAIL PROTECTED] - http://www.xs4all.nl/~hanwen
Re: the new gc asserts in master
Andy Wingo escreveu: > Hi, > > I just merged master to guile-vm, but I'm not sure if I really wanted to > do that now. Normal test suites are failing: > > lt-guile: gc.c:604: scm_i_gc: Assertion `scm_cells_allocated == > scm_i_marked_count ()' failed. > /home/wingo/src/guile/vm/test-suite/standalone/test-use-srfi: line 27: > 29507 Aborted guile -q --use-srfi=1,10 > /dev/null < (if (and (defined? 'partition) > (defined? 'define-reader-ctor)) > (exit 0) ;; good > (exit 1)) ;; bad > EOF > > guile --use-srfi=1,10 fails to run > FAIL: test-use-srfi > > This is on a core 2 duo, in 32-bit mode, configured as: > > CFLAGS="-g -O2" ./configure --with-threads --enable-maintainer-mode > --prefix=/opt/guile-vm > > So it seems that the new gc "cleanups" don't want you to touch mark bits > outside the mark phase. This is incompatible with other uses inside > guile itself, e.g. the SCM_DEBUG_CELL_ACCESSES == 1 case in inline.h, or I am pushing a fix for this to master. > even the lazy smob case I wrote about here: > > http://thread.gmane.org/gmane.lisp.guile.user/6372 I would classify the use of mark bits outside of the mark phase as outside of the defined API. If you want to have weak pointer semantics, use a weak hashtable, or implement reference counting on the C side. I am actuallly inclined to add add abort() for anyone who calls scm_gc_mark() outside the marking phase. > There are more cases, rgrep for SCM_SET_GC_MARK in libguile/*.[ch]. I looked through all of these, and these all happen during the mark phase. > I'm going to commit an #if 0 around those asserts in the vm branch, > because I don't have the brain power to deal with it. (It is irritating > that I have to even write this mail.) Certainly if the near-term choice > is between inaccurate statistics and calls to abort(), I know which > choice I prefer... The statistics form the basis of the allocation strategy, so they are not a 'cute' feature. If these statistics go off, we overallocate, or spend too much time trying garbage collect when there is nothing to reclaim. -- Han-Wen Nienhuys - [EMAIL PROTECTED] - http://www.xs4all.nl/~hanwen
