sly-inspector: keeps a refernece to data even after its been closed

[mariari]

The Sly-inspector while a very useful tool for looking at hashtable contents, does not do a good job at respecting the weakness of the hashtable

Here is a transcript

CL> (defparameter *x* (make-hash-table :test #'eq
                                       :weakness :key))
*X*
CL> (setf (gethash (tg:make-weak-pointer :a) *x*) 5)
5 (3 bits, #x5, #o5, #b101)
CL> *x*
#<SB-IMPL::GENERAL-HASH-TABLE :TEST EQ :COUNT 1 :WEAKNESS :KEY {1005F529F3}>
CL> (tg:gc :full t)
NIL
CL> *x*
#<SB-IMPL::GENERAL-HASH-TABLE :TEST EQ :COUNT 0 :WEAKNESS :KEY {10013B8073}>
CL> (setf (gethash (tg:make-weak-pointer :a) *x*) 5)
5 (3 bits, #x5, #o5, #b101)
CL> *x*
#<SB-IMPL::GENERAL-HASH-TABLE :TEST EQ :COUNT 1 :WEAKNESS :KEY {10013B8073}>

;; I now open up the inspector

;; I now quit the inspector

CL> (tg:gc :full t)
NIL
CL> *x*
#<SB-IMPL::GENERAL-HASH-TABLE :TEST EQ :COUNT 1 :WEAKNESS :KEY {10013A0073}>

As we can see, if we insert a weak pointer into the hashtable all is fine, after we reclaim the hashtable is back down to 0 values.

However, if we open up the inspector on the hashtable it ceases to garbage collect the weak pointers.

This can cause some memory leaks, and in my system has made me tried to track down where a live reference may live.

It would be nice, if the inspector were closed if it could remove references to the data, and or just make the inspected view of a weak-hashtable have weak pointers to data with the same policy as the hashtable itself

[joaotavora]

Yes, i think you're right, though i haven't looked at this code in a long time. I think it has to do with the history feature of the inspector, which allows you to go back. But I don't understand exactly what you mean by opening the inspector and closing it. Can you be perfectly specific as to what you do in those two steps. A reference to the weak hash table itself should not prevent scavenging of its entries. That would need a reference to one of the weak objects in the entry.

[mariari]

Sure, by open the inspector I call

sly-button-inspect

and by close I mean either

sly-inspector-quit or kill-buffer

Once one of those actions happen, the keys in the table itself isn't able to be scavenged (I have not tested with other weakness criterias)

#<SB-IMPL::GENERAL-HASH-TABLE {10013A0073}>
--------------------
Count: 1
Size: 7
Test: EQ
Rehash size: 1.5
Rehash threshold: 1.0
Weakness:: :KEY
[clear hashtable]
Contents: 
#<weak pointer: :A> = 5 [remove entry]

This is the inspector menu I see, when I open the inspector for reference

[joaotavora]

Reproduced in SBCL with sb-ext:make-weak-pointer as the only tweak to the original recipe.

As we can see if we evaluate (inspector-%history (current-inspector)) in the :slynk package, the inspection history is keeping the inspection body's contents as well.

#(#S(ISTATE
     :OBJECT #<SB-IMPL::GENERAL-HASH-TABLE :TEST EQ :COUNT 0 :WEAKNESS :KEY {10011B9993}>
     :PARTS #(#<SB-IMPL::GENERAL-HASH-TABLE :TEST EQ :COUNT 0 :WEAKNESS :KEY {10011B9993}>
              0 7 EQ 1.5 1.0 :KEY)
     :ACTIONS #()
     :METADATA NIL
     :CONTENT ("Count" ": " (:VALUE 0) (:NEWLINE) "Size" ": " (:VALUE 7)
               (:NEWLINE) "Test" ": " (:VALUE EQ) (:NEWLINE) "Rehash size" ": "
               (:VALUE 1.5) (:NEWLINE) "Rehash threshold" ": " (:VALUE 1.0)
               (:NEWLINE) "Weakness:" ": " (:VALUE :KEY) (:NEWLINE))
     :SERIAL NIL)
  #S(ISTATE
     :OBJECT #<SB-IMPL::GENERAL-HASH-TABLE :TEST EQ :COUNT 1 :WEAKNESS :KEY {10010A8073}>
     :PARTS #(#<SB-IMPL::GENERAL-HASH-TABLE :TEST EQ :COUNT 1 :WEAKNESS :KEY {10010A8073}>
              1 7 EQ 1.5 1.0 :KEY #<weak pointer: :A> 5)
     :ACTIONS #((#<FUNCTION (LAMBDA () :IN EMACS-INSPECT) {1001934ABB}> T)
                (#<FUNCTION (LAMBDA () :IN EMACS-INSPECT) {1001934ADB}> T))
     :METADATA NIL
     :CONTENT ("Count" ": " (:VALUE 1) (:NEWLINE) "Size" ": " (:VALUE 7)
               (:NEWLINE) "Test" ": " (:VALUE EQ) (:NEWLINE) "Rehash size" ": "
               (:VALUE 1.5) (:NEWLINE) "Rehash threshold" ": " (:VALUE 1.0)
               (:NEWLINE) "Weakness:" ": " (:VALUE :KEY) (:NEWLINE)
               (:ACTION "[clear hashtable]"
                #<FUNCTION (LAMBDA () :IN EMACS-INSPECT) {1001934ABB}>)
               (:NEWLINE) "Contents: " (:NEWLINE) (:VALUE #<weak pointer: :A>)
               " = " (:VALUE 5) " "
               (:ACTION "[remove entry]"
                #<FUNCTION (LAMBDA () :IN EMACS-INSPECT) {1001934ADB}>)
               (:NEWLINE))
     :SERIAL NIL))

I tend to think it shouldn't. The reference to the inspectee makes sense, but the contents should regenerated on each navigation.

Keeping the history of objects inspected in the past is a nice feature but this needs to be reworked.

[joaotavora]

Hi @mariari I've added three commits in a side branch fix/568-dont-prevent-gc-from-inspector-references that should fix this issue for you.

However:

• Though the bug existed, your expectation that a simple sly-inspector-quit (or pressing q) from the inspector buffer relinquishes the references is not correct. After getting re-acquainted with the code, I noticed that I had thought about this (or someone did). You must ask for the inspector to be reset -- i.e. to lose its history. This is done by passing a prefix argument to sly-inspector-quit, or just typing C-u q instead of q.

• Killing the buffer does not clear the history (and so does not clear the references) meaning you can still get to old inspected elements. The goal here is, as usual in SLY, to err on the side of debuggin convenience, but give you the power to discard references.

• There was indeed a bug, and I haven't been able to track down the root cause yet. It's as if arrays (or at least that particular adjustible array) count as strong references even if noone holds them. Or mmore likely that array is being held somewhere else I that I'm not seeing.

• You don't need the make-weak-pointer there in your recipe. You're just making two weak pointers, it's probably easier to just insert a (gensym) as a key in the weak hash table to demo the bug.

• I tested this on SBCL where I saw the bug. I would like you to try C-u q before trying out the patch. Maybe the GC issue I saw is SBCL specific :shrug:

[joaotavora]

Indeed, there appears to be some SBCL-specifics here, as Allegro Common Lisp doesn't require the commit https://github.com/joaotavora/sly/commit/a432b253812871a01c2c77ff54464b2406242af9 for the C-u q technique to work and gc to work.

[mariari]

Indeed, there appears to be some SBCL-specifics here, as Allegro Common Lisp doesn't require the commit a432b25 for the C-u q technique to work and gc to work.

I just confirmed that for ccl no patch is needed to make C-u q work as well.