Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#704 closed defect (fixed)

eql-specializer-objects are not freed (remove-method)

Reported by: lnostdal Owned by:
Priority: normal Milestone:
Component: Runtime (threads, GC) Version: trunk
Keywords: Cc:

Description (last modified by gb)

Just tried this https://bugs.launchpad.net/sbcl/+bug/492851 in CCL and got the same result after a while.

Here is the code:

(defgeneric blah (x))

(defun test ()
  (let ((object (make-list 10000)))
    (defmethod blah ((x (eql object)))
      (format t "~A~%" x))
    (remove-method #'blah (first (generic-function-methods #'blah))))
  (values))

Then do:

(loop (test))

..and memory use keeps rising.

Change History (5)

comment:1 follow-up: Changed 9 years ago by gb

  • Resolution set to fixed
  • Status changed from new to closed

(In [13980]) Make the hash table used by INTERN-EQL-SPECIALIZER weak on value. Don't call RECORD-SOURCE-FILE in ENSURE-METHOD; do call it from the expansion of DEFMETHOD. (Could do so more concisely, but that involves a little bit of bootstrapping.) Fixes ticket:704.

comment:2 Changed 9 years ago by gb

  • Description modified (diff)

The mapping from objects to EQL specializers on those objects should be weak (if the specializer isn't otherwise referenced, it and the corresponding object can be GCed.)

There's another issue here. A defining macro like DEFMETHOD can expand into code which has arbitrary implementation-dependent execution-time effects (keeping track of source information or other debugging info) as well as the defined effects (ENSURE-METHOD or something equivalent, etc.) There's at least some argument that says that the benefits of retaining debugging information about otherwise unreferenced obsolete methods (and having that retention prevent the methods and/or their specializers from being GCed) outweigh the disadvantages of doing so. (I'm not sure that I believe that; I don't have too much good to say about how CCL handles source-location recording, but there's at least an argument there.)

Rather than (ab)using a defining-macro (DEFMETHOD) to do method redefinition declaratively at runtime, it should be possible to do this procedurally (via ENSURE-METHOD) and not worry about those other effects. It wasn't possible prior to r13980 in the trunk; ENSURE-METHOD was calling CCL::RECORD-SOURCE-FILE, and that should really happen elsewhere in the expansion of DEFMETHOD.

In other words, if your example used ENSURE-METHOD, I think that its memory utilization should be relatively stable and AFAICT it is in the CCL trunk as of r13980.) If the example uses DEFMETHOD, it's not totally unreasonable that environmental/debugging info accrues, and that information costs something; someone who finds that information more useful than I do might consider that to be a reasonable tradeoff in many cases.

comment:3 in reply to: ↑ 1 ; follow-up: Changed 9 years ago by lnostdal

Replying to gb:

(In [13980]) Make the hash table used by INTERN-EQL-SPECIALIZER weak on value. Don't call RECORD-SOURCE-FILE in ENSURE-METHOD; do call it from the expansion of DEFMETHOD. (Could do so more concisely, but that involves a little bit of bootstrapping.) Fixes ticket:704.

I keep thinking using a weak key hash-table will lead to bignums (which are IIUC possible to compare using EQL) being GCed too aggressively?

comment:4 in reply to: ↑ 3 ; follow-up: Changed 9 years ago by gb

Replying to lnostdal:

Replying to gb:

(In [13980]) Make the hash table used by INTERN-EQL-SPECIALIZER weak on value. Don't call RECORD-SOURCE-FILE in ENSURE-METHOD; do call it from the expansion of DEFMETHOD. (Could do so more concisely, but that involves a little bit of bootstrapping.) Fixes ticket:704.

I keep thinking using a weak key hash-table will lead to bignums (which are IIUC possible to compare using EQL) being GCed too aggressively?

The hash table in question maps arbitrary objects to EQL-SPECIALIZERs via EQL and is now weak on value (not weak on key.) This means that when the value (the EQL-SPECIALIZER) is otherwise unreferenced, the key/value pair is removed from the hash table and can be GCed. The specializer generally becomes otherwise unreferenced after the containing method is removed from the GF.

I can't quite parse the rest of your comment, but if you're saying that it wouldn't work to have an EQL hash table be weak on key ... that's certainly true, but not relevant here.

comment:5 in reply to: ↑ 4 Changed 9 years ago by lnostdal

Replying to gb:

The hash table in question maps arbitrary objects to EQL-SPECIALIZERs via EQL and is now weak on value (not weak on key.)

Whops. Yeah, never mind.

Note: See TracTickets for help on using tickets.