From: Douglas K. <do...@go...> - 2013-09-13 18:40:28
|
Hi, Cold-print accidentally didn't print the recursed-into object in its fallback case, but instead the outer argument. Hexstr didn't work on 64-bit. Both for "hapless wretches" [sic]. Re my prior submittal: - Yes I know I forgot an (IN-PACKAGE) at the top of the test file. And I have some tests which bash on it a little harder now. - Some improvements, pending, make the code less aesthetically displeasing. - If the volatile-env/compact-env distinction is to be kept, that's fine, but we can reduce the memory used by ~3x. Volatile env entries cost 4 words each - a cons for the entry and a cons to push it. Vectors cost 4 words plus ~1.2 words per entry in my packing. Breakeven = one entry. So replace the cdr of the name's cell with a vector and it's an immediate win- performance-wise also. Ok, this leads to my question: Given a patched Lisp, _that_ won't build itself. I think I need another piece of voodoo somewhere and so to find if I accidentally depended on something in the host Lisp, I tried building on CCL. This turns out not to work, mostly often because CCL considers it an error when a TYPECASE expression contains an unmatchable clause. These arise in many places: 'compiler/dump.lisp' forgets the idiom of "#-sb-xc-host" in front of the branches that might not have a specialization. (see "KLUDGE: This isn't the best way of expressing that the host may not have specializations") So this merits fixing at least for consistency's sake. Also this applies universally to Paul's apparently favored approach of: #!+sb-simd-pack (#+sb-xc-host nil #-sb-xc-host (simd-pack double-float) (sc-number-or-lose 'double-sse-immediate)) which says that if the target wants the simd-pack feature but we're building the xc the whole clause is nil. I'm not sure if this is a bug in CCL that it rejects those. Anyway does it sound plausible that I might need to add another crossed/uncrossed function? Patch to the cold print for your enjoyment. --- a/src/code/cold-init.lisp +++ b/src/code/cold-init.lisp @@ -370,20 +370,21 @@ process to continue normally." #!+sb-show (defun hexstr (thing) (/noshow0 "entering HEXSTR") - (let ((addr (get-lisp-obj-address thing)) - (str (make-string 10 :element-type 'base-char))) + (let* ((addr (get-lisp-obj-address thing)) + (nchars (* sb!vm:n-word-bytes 2)) + (str (make-string (+ nchars 2) :element-type 'base-char))) (/noshow0 "ADDR and STR calculated") (setf (char str 0) #\0 (char str 1) #\x) (/noshow0 "CHARs 0 and 1 set") - (dotimes (i 8) + (dotimes (i nchars) (/noshow0 "at head of DOTIMES loop") (let* ((nibble (ldb (byte 4 0) addr)) (chr (char "0123456789abcdef" nibble))) (declare (type (unsigned-byte 4) nibble) (base-char chr)) (/noshow0 "NIBBLE and CHR calculated") - (setf (char str (- 9 i)) chr + (setf (char str (- (+ nchars 1) i)) chr addr (ash addr -4)))) str)) @@ -403,6 +404,6 @@ process to continue normally." (%cold-print (car obj) d) (%cold-print (cdr obj) d))) (t - (sb!sys:%primitive print (hexstr x))))))) + (sb!sys:%primitive print (hexstr obj))))))) |
From: Christophe R. <cs...@ca...> - 2013-10-07 12:36:20
|
Douglas Katzman <do...@go...> writes: > Cold-print accidentally didn't print the recursed-into object in its > fallback case, but instead the outer argument. Hexstr didn't work on > 64-bit. Both for "hapless wretches" [sic]. Thanks. I've incorporated your fixes (by hand, sorry, so there may be conflicts). I'm not ignoring the big globaldb issue, but I have slightly lost track, partly because as a hapless wretch you have managed to trigger issue after issue :-) so maybe we could recap -- is the current status clearer now? The message I'm replying to refers to an inability to self-rebuild; has that been fixed? What's the latest thing that I should be reviewing? Thanks, Christophe |
From: Douglas K. <do...@go...> - 2013-10-07 15:37:18
|
Hi Christophe, thanks for mulling over my changesets. The email saying "Nice compiler speedup" on Sep 17th contains a completely working patch. I have minor touch-ups that are not strictly needed, and I mean really minor - most notably a comment containing a prototypical packed info representation is wrong, because I changed the representation without pasting in a new comment. I could mail you a complete patch with touch-ups; but to keep things simple, you could review the patch as-is and provide feedback. No need to stir the pot any more at this point. If it's all the same to you, that patch hangs together as one conceptual thing, however it does have sub-pieces that can be factored out if you like: * The non-self-rebuild was my fault which of course I needed a working cold-print to track down. The aforementioned patch can self-rebuild fine. It was a crash on the road to getting StaticSymbolFunction() not to be special-cased. As you know, that C code currently only works for symbols whose fdefinition is in their value cell (as well as globaldb). My representation facilitated removing that limitation, so I did. Technically I needn't have touched C code at all though, if only I had preserved the behavior that WHN deemed "weird". * WHN's comment that it is ugly that DEFINE-INFO-TYPE takes a :DEFAULT form which assumes the existence of a lexical var named NAME, which may be used to decide what an info's default value should be. This was somewhat orthogonal to anything else, but I felt like correcting the lack of hygiene. * remark, sans fix, that SETF-ASSUMED-FBOUNDP is a bit silly - just a few lines to add a comment, so why not? ("Monsieur, it's wafer thin ...") You can stop reading now and just go review that (thanks!), but here are remaining issues I came across though, none of which are incorporated into the large patch, and some of which I sent subsequent emails about: - The fact that clear-info-value doesn't work means that the second return value of GET-INFO-VALUE (aka the WINP/FOUNDP value) is genuinely wrong sometimes, because at best you can set a value back to NIL, but not truly clear it from a compact env. If the big patch is done along with my subsequent proposal to use a quasi-lock-free unified hashtable for "complicated" names, we can then go through 'info-functions' and see what else should have been cleared instead of set to NIL. e.g. the logic in UNDEFINE-FUN-NAME in src/compiler/info-functions *knows* that clearing doesn't actually work as advertised, and it's secretly using the workaround without saying so. - The general issue (which I never emailed to sbcl-devel but only in conversation with Nikodemus) of method names that have an EQL in them generate several different "names" in globaldb - potentially the cross-product of each of {'fast-method,'slow-method} with the value of the EQL form {before, after} it has been evaled. We should eradicate all infos from bogus names. One way to do that is that method functions should be anonymous lambdas until the putative name of such lambda has had all (if any) EQL specializers interned, because otherwise every source-form used to define every EQL specializer is needlessly part of a (useless) name in globaldb. - The poor hashing of list-shaped PCL method names due to sxhashoid's depthoid cutting off in both the CAR and CDR axis (with or without a fix to the preceding issue, this is still an issue) - Leftover detritus from cross-compilation remains in target Lisp as the :xc-constant-value info-type. I have made that not exist in the target. - A comment "FIXME: is it too expensive to go through a runtime call to FDEFINITION each time we want to check a name's syntax?" can be dealt with by storing the function that computes a name's validity, not the symbol naming the function that computes the name's validity. There's a bootstrapping issue afaict. Note that my implementation of fdefinition-object avoids calling legal-fun-name-or-type-error until it absolutely has to due to a "miss" in the globaldb for the fdefinition of the name being sought. [Also before anyone mentions that the hashtable algorithm (in the email of Oct 1) doesn't support key removal: indeed, but you don't need key removal, you only need removal of an info-type-number which would be in the packed-info-vector stored under a key's value, and that is supported by, in the limiting case, having a vector of no info.] I hope all is clear now. Regards. -d On Mon, Oct 7, 2013 at 8:36 AM, Christophe Rhodes <cs...@ca...> wrote: > Douglas Katzman <do...@go...> writes: > > > Cold-print accidentally didn't print the recursed-into object in its > > fallback case, but instead the outer argument. Hexstr didn't work on > > 64-bit. Both for "hapless wretches" [sic]. > > Thanks. I've incorporated your fixes (by hand, sorry, so there may be > conflicts). > > I'm not ignoring the big globaldb issue, but I have slightly lost track, > partly because as a hapless wretch you have managed to trigger issue > after issue :-) so maybe we could recap -- is the current status clearer > now? The message I'm replying to refers to an inability to > self-rebuild; has that been fixed? What's the latest thing that I > should be reviewing? > > Thanks, > > Christophe > |