From: Kalyanov D. <kal...@gm...> - 2010-12-26 19:48:21
|
I'd like to consider the possibility of merging Windows threads support into SBCL main tree. I've put up page with information about threads support at http://dmitryvk.github.com/sbcl-win32-threads/ (including links to the code). How should I proceed? Should I chop this patch to smaller pieces or can it be applied as one big patch? Safepoints may also be useful for threading on other platforms - should safepoints be ported first or it's better to refactored them later? I'd like to hear any advice. |
From: Alastair B. <ala...@gm...> - 2010-12-29 23:28:07
|
I have a few questions about this this branch that I'd like to see answered prior to merging. The enter/leave region functions seem... ungraceful, somehow. There is a lot of repetitive code that could be papered over via a MACROLET, the function names are ugly in context, and the /no-fixup bit could just as easily be an optional or keyword parameter. But there's one further possibility: Have you considered using an instruction-macro, so that the call site becomes something on the order of (inst enter-safe-region :no-fixup) ? In the definition for +win32-tib-arbitrary-field-offset+, there is no commentary as to what this offset corresponds to, it ends up just being a magic number. Can this reasonably be added, along with a justification for why hacking up an arbitrary field in the TIB is "okay"? (I see that this is explained elsewhere, but there's nothing to link the definition to the explanation... or vice versa.) Why emulate pthreads and posix signal handling instead of doing something more "windows-like"? Why even have definitions for signals other than SIG_STOP_FOR_GC and SIGPIPE (for interrupt-thread), and possibly SIGINT (for use by SetConsoleCtrlHandler() handlers)? Those are the major questions, to my mind, at least. On Sun, Dec 26, 2010 at 2:48 PM, Kalyanov Dmitry <kal...@gm...> wrote: > I'd like to consider the possibility of merging Windows threads support > into SBCL main tree. I've put up page with information about threads > support at http://dmitryvk.github.com/sbcl-win32-threads/ (including > links to the code). > > How should I proceed? Should I chop this patch to smaller pieces or can > it be applied as one big patch? Safepoints may also be useful for > threading on other platforms - should safepoints be ported first or it's > better to refactored them later? Smaller pieces? Definitely! Consider the linux/ppc threading changes: Some 40 separate commits in the 1.0.41.x series (essentially, almost all of the commits I made for that release were related to ppc threading). Integrating safepoints first? Yes, probably: It's plausibly useful on other platforms (perhaps making it work on linux would be a good start), and constitutes a "smaller piece", although it should probably be split up into more than one commit as well. > I'd like to hear any advice. -- Alastair Bridgewater |
From: Roman M. <rom...@gm...> - 2010-12-30 13:16:16
|
It would be nice to remove other magic numbers from the patch as well. I have seen several "4" hardcoded in the patch (mostly in SB-VM part of it). It looks like the 32-bit pointer size, but this is not a justification. In general (not in this patch), magic numbers are quite common in SBCL's SB-VM code, and this is a problem. When I look at this code from call.lisp, I wonder why don't we name the things properly? (+ (if named 5 0) (if variable 19 1) (if (eq return :tail) 0 10) 15 (if (eq return :unknown) 25 0)) Regards, Roman 2010/12/30 Alastair Bridgewater <ala...@gm...> > > In the definition for +win32-tib-arbitrary-field-offset+, there is no > commentary as to what this offset corresponds to, it ends up just > being a magic number. > > |
From: Paul K. <pv...@pv...> - 2010-12-30 14:47:49
|
On 2010-12-30, at 8:16 AM, Roman Marynchak wrote: > It would be nice to remove other magic numbers from the patch as well. I > have seen several "4" hardcoded in the patch (mostly in SB-VM part of it). > It looks like the 32-bit pointer size, but this is not a justification. > > In general (not in this patch), magic numbers are quite common in SBCL's > SB-VM code, and this is a problem. When I look at this code from call.lisp, > I wonder why don't we name the things properly? > > (+ (if named 5 0) > (if variable 19 1) > (if (eq return :tail) 0 10) > 15 > (if (eq return :unknown) 25 0)) I don't even have to look at the source to tell that this is computing a VOP's cost. Those are currently almost arbitrary, except for the total order between equally applicable VOPs. Frankly, I have no idea how one would give a name to these value, except for the obvious name from the condition they depend on. Paul Khuong |
From: Pascal J. B. <pj...@in...> - 2010-12-30 14:59:51
|
Roman Marynchak <rom...@gm...> writes: > It would be nice to remove other magic numbers from the patch as well. I > have seen several "4" hardcoded in the patch (mostly in SB-VM part of > it). It looks like the 32-bit pointer size, but this is not a > justification. > > In general (not in this patch), magic numbers are quite common in SBCL's > SB-VM code, and this is a problem. When I look at this code from > call.lisp, I wonder why don't we name the things properly? > > (+ (if named 5 0) > (if variable 19 1) > (if (eq return :tail) 0 10) > 15 > (if (eq return :unknown) 25 0)) Without mentionning the indentation... (+ (if named 5 0) (if variable 19 1) (if (eq return :tail) 0 10) 15 (if (eq return :unknown) 25 0)) and unless return is a symbol-macro with side effects, (+ 15 (if named 5 0) (if variable 19 1) (case return ((:tail) 0) ((:unknown) 35) (otherwise 10))) -- __Pascal Bourguignon__ http://www.informatimago.com/ A bad day in () is better than a good day in {}. |