code review 198085: finalizers; merge package malloc into package runtime

finalizers; merge package malloc into package runtime

[rsc, 2010-02-03 07:16:32]

Hello r (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[r2, 2010-02-03 09:10:34]

is there content to this? if it's just an mv followed by renames, LGTM

[rsc, 2010-02-03 09:23:57]

The changes in src/pkg/runtime are significant
(they implement finalizers), and test/mallocfin.go is a test.
The files outside src/pkg/runtime are trivial renaming changes.

Russ

[r, 2010-02-03 09:42:08]

http://codereview.appspot.com/198085/diff/71/78
File src/pkg/runtime/cgo2c.c (right):

http://codereview.appspot.com/198085/diff/71/78#newcode49
src/pkg/runtime/cgo2c.c:49: Eface,
what is an Eface?

http://codereview.appspot.com/198085/diff/71/79
File src/pkg/runtime/extern.go (right):

http://codereview.appspot.com/198085/diff/71/79#newcode106
src/pkg/runtime/extern.go:106: // finalizer with x, the next garbage collection
round will free x.
what if the finalizer is still running?  or to put it another way, define 'next'

http://codereview.appspot.com/198085/diff/71/79#newcode124
src/pkg/runtime/extern.go:124: // Finalizers should be used sparingly, usually
to release non-memory
you can't really say this here unless you explain why. it's either supported or
it's not.  people will use this feature (it's my main objection to it) whatever
you say here. give fair warning about what's really wrong with doing it.

pardon me. people will not use the feature, they will abuse it.

http://codereview.appspot.com/198085/diff/71/79#newcode126
src/pkg/runtime/extern.go:126: // a finalizer that closes the associated
operating system file descriptor.
it does?

http://codereview.appspot.com/198085/diff/71/82
File src/pkg/runtime/mfinal.c (right):

http://codereview.appspot.com/198085/diff/71/82#newcode26
src/pkg/runtime/mfinal.c:26: addkeyval(Fintab *t, void *k, void *v)
this seems like the wrong function name. it should at least have fin or Fin in
it somewhere. (think about what crash dumps will look like)

http://codereview.appspot.com/198085/diff/71/82#newcode34
src/pkg/runtime/mfinal.c:34: goto ret;
yuck.
how about at least moving ret: to the end of the function?

http://codereview.appspot.com/198085/diff/71/82#newcode52
src/pkg/runtime/mfinal.c:52: lookkey(Fintab *t, void *k, bool del)
name

http://codereview.appspot.com/198085/diff/71/83
File src/pkg/runtime/mgc0.c (right):

http://codereview.appspot.com/198085/diff/71/83#newcode178
src/pkg/runtime/mgc0.c:178: if(Debug)
sometimes you say Debug and sometimes you say Debug > 1

[r2, 2010-02-03 09:42:34]

not sure why it's not two CLs but OK

On 03/02/2010, at 8:23 PM, Russ Cox wrote:

> The changes in src/pkg/runtime are significant
> (they implement finalizers), and test/mallocfin.go is a test.
> The files outside src/pkg/runtime are trivial renaming changes.
> 
> Russ
> 

[cw, 2010-02-03 09:50:44]

Can this be broken up at all?  Maybe it's just me, but using Rietveld to eyeball
something this large in a browser is painful.

http://codereview.appspot.com/198085/diff/71/80
File src/pkg/runtime/malloc.cgo (right):

http://codereview.appspot.com/198085/diff/71/80#newcode284
src/pkg/runtime/malloc.cgo:284: func SetFinalizer(x Eface, f Eface) {
silly nit, but x & f are very terse tokens here which makes using browser
searching/highlighting pointless when trying to see how they are used

i also though we might have a case where if f was nil it would 'unset' the
finalizer

http://codereview.appspot.com/198085/diff/71/80#newcode312
src/pkg/runtime/malloc.cgo:312: printf("runtime.SetFinalizer: finalizer already
set");
i thought it was agreed subsequent calls would replace the finalizer?

[r2, 2010-02-03 10:02:15]

On 03/02/2010, at 8:50 PM, cw@f00f.org wrote:

> Can this be broken up at all?  Maybe it's just me, but using Rietveld to
> eyeball something this large in a browser is painful.

One acclimates.  A tab per file is helpful.

-rob

[rsc, 2010-02-03 10:05:06]

On Wed, Feb 3, 2010 at 01:50, <cw@f00f.org> wrote:

> Can this be broken up at all?  Maybe it's just me, but using Rietveld to
> eyeball something this large in a browser is painful.
>

if you're a patch kind of guy there's always "Download raw patch file".

[rsc, 2010-02-03 10:05:31]

http://codereview.appspot.com/198085/diff/71/78
File src/pkg/runtime/cgo2c.c (right):

http://codereview.appspot.com/198085/diff/71/78#newcode49
src/pkg/runtime/cgo2c.c:49: Eface,
On 2010/02/03 09:42:08, r wrote:
> what is an Eface?

It's an empty interface; it's a C type defined in the runtime code, like Slice
and String.

http://codereview.appspot.com/198085/diff/71/79
File src/pkg/runtime/extern.go (right):

http://codereview.appspot.com/198085/diff/71/79#newcode124
src/pkg/runtime/extern.go:124: // Finalizers should be used sparingly, usually
to release non-memory
Reworded.  PTAL

http://codereview.appspot.com/198085/diff/71/79#newcode126
src/pkg/runtime/extern.go:126: // a finalizer that closes the associated
operating system file descriptor.
On 2010/02/03 09:42:08, r wrote:
> it does?

not yet but that will be the next step.

http://codereview.appspot.com/198085/diff/71/80
File src/pkg/runtime/malloc.cgo (right):

http://codereview.appspot.com/198085/diff/71/80#newcode284
src/pkg/runtime/malloc.cgo:284: func SetFinalizer(x Eface, f Eface) {
On 2010/02/03 09:50:44, cw wrote:
> silly nit, but x & f are very terse tokens here which makes using browser
> searching/highlighting pointless when trying to see how they are used

fixed

> i also though we might have a case where if f was nil it would 'unset' the
> finalizer

the call to addfinalizer does that; if f == nil, finalizer.data == nil.

http://codereview.appspot.com/198085/diff/71/80#newcode312
src/pkg/runtime/malloc.cgo:312: printf("runtime.SetFinalizer: finalizer already
set");
On 2010/02/03 09:50:44, cw wrote:
> i thought it was agreed subsequent calls would replace the finalizer?

It seemed less error prone to scream loudly if two
different pieces of code are fighting over the finalizer.

If you really need to reset it you can always SetFinalizer
to nil first and then to a real pointer.  Then at least it's
clear that you're trying to overwrite an existing one.

http://codereview.appspot.com/198085/diff/71/82
File src/pkg/runtime/mfinal.c (right):

http://codereview.appspot.com/198085/diff/71/82#newcode26
src/pkg/runtime/mfinal.c:26: addkeyval(Fintab *t, void *k, void *v)
On 2010/02/03 09:42:08, r wrote:
> this seems like the wrong function name. it should at least have fin or Fin in
> it somewhere. (think about what crash dumps will look like)

Done.  (And the caller will be in the crash dump too.)

http://codereview.appspot.com/198085/diff/71/82#newcode34
src/pkg/runtime/mfinal.c:34: goto ret;
On 2010/02/03 09:42:08, r wrote:
> yuck.
> how about at least moving ret: to the end of the function?

Done.

http://codereview.appspot.com/198085/diff/71/82#newcode52
src/pkg/runtime/mfinal.c:52: lookkey(Fintab *t, void *k, bool del)
On 2010/02/03 09:42:08, r wrote:
> name

Done.

http://codereview.appspot.com/198085/diff/71/83
File src/pkg/runtime/mgc0.c (right):

http://codereview.appspot.com/198085/diff/71/83#newcode178
src/pkg/runtime/mgc0.c:178: if(Debug)
On 2010/02/03 09:42:08, r wrote:
> sometimes you say Debug and sometimes you say Debug > 1

changed former to Debug > 0.

[cw, 2010-02-03 10:07:31]

On Wed, Feb 03, 2010 at 02:05:03AM -0800, Russ Cox wrote:

> if you're a patch kind of guy there's always "Download raw patch
> file".

i ended up doing that, less is wonderful, tabs less so :-)

[cw, 2010-02-03 10:11:52]

On Wed, Feb 03, 2010 at 07:16:32AM +0000, rsc@golang.org wrote:

> Please review this at http://codereview.appspot.com/198085/show

Should this work as-is?

I just applied this and tried it out in a contrived way and didn't see
my finalizer do anything after releasing references and calling
runtime.GC().

What's more, can't a finalizer be

  func myfinalizer(x interface{}) { }

or similar so i can potentially use the same function for different
types?

[r2, 2010-02-03 10:12:33]

upload?

On 03/02/2010, at 9:05 PM, rsc@golang.org wrote:

> 
> http://codereview.appspot.com/198085/diff/71/78
> File src/pkg/runtime/cgo2c.c (right):
> 
> http://codereview.appspot.com/198085/diff/71/78#newcode49
> src/pkg/runtime/cgo2c.c:49: Eface,
> On 2010/02/03 09:42:08, r wrote:
>> what is an Eface?
> 
> It's an empty interface; it's a C type defined in the runtime code, like
> Slice and String.
> 
> http://codereview.appspot.com/198085/diff/71/79
> File src/pkg/runtime/extern.go (right):
> 
> http://codereview.appspot.com/198085/diff/71/79#newcode124
> src/pkg/runtime/extern.go:124: // Finalizers should be used sparingly,
> usually to release non-memory
> Reworded.  PTAL
> 
> http://codereview.appspot.com/198085/diff/71/79#newcode126
> src/pkg/runtime/extern.go:126: // a finalizer that closes the associated
> operating system file descriptor.
> On 2010/02/03 09:42:08, r wrote:
>> it does?
> 
> not yet but that will be the next step.
> 
> http://codereview.appspot.com/198085/diff/71/80
> File src/pkg/runtime/malloc.cgo (right):
> 
> http://codereview.appspot.com/198085/diff/71/80#newcode284
> src/pkg/runtime/malloc.cgo:284: func SetFinalizer(x Eface, f Eface) {
> On 2010/02/03 09:50:44, cw wrote:
>> silly nit, but x & f are very terse tokens here which makes using
> browser
>> searching/highlighting pointless when trying to see how they are used
> 
> fixed
> 
>> i also though we might have a case where if f was nil it would 'unset'
> the
>> finalizer
> 
> the call to addfinalizer does that; if f == nil, finalizer.data == nil.
> 
> http://codereview.appspot.com/198085/diff/71/80#newcode312
> src/pkg/runtime/malloc.cgo:312: printf("runtime.SetFinalizer: finalizer
> already set");
> On 2010/02/03 09:50:44, cw wrote:
>> i thought it was agreed subsequent calls would replace the finalizer?
> 
> It seemed less error prone to scream loudly if two
> different pieces of code are fighting over the finalizer.
> 
> If you really need to reset it you can always SetFinalizer
> to nil first and then to a real pointer.  Then at least it's
> clear that you're trying to overwrite an existing one.
> 
> http://codereview.appspot.com/198085/diff/71/82
> File src/pkg/runtime/mfinal.c (right):
> 
> http://codereview.appspot.com/198085/diff/71/82#newcode26
> src/pkg/runtime/mfinal.c:26: addkeyval(Fintab *t, void *k, void *v)
> On 2010/02/03 09:42:08, r wrote:
>> this seems like the wrong function name. it should at least have fin
> or Fin in
>> it somewhere. (think about what crash dumps will look like)
> 
> Done.  (And the caller will be in the crash dump too.)
> 
> http://codereview.appspot.com/198085/diff/71/82#newcode34
> src/pkg/runtime/mfinal.c:34: goto ret;
> On 2010/02/03 09:42:08, r wrote:
>> yuck.
>> how about at least moving ret: to the end of the function?
> 
> Done.
> 
> http://codereview.appspot.com/198085/diff/71/82#newcode52
> src/pkg/runtime/mfinal.c:52: lookkey(Fintab *t, void *k, bool del)
> On 2010/02/03 09:42:08, r wrote:
>> name
> 
> Done.
> 
> http://codereview.appspot.com/198085/diff/71/83
> File src/pkg/runtime/mgc0.c (right):
> 
> http://codereview.appspot.com/198085/diff/71/83#newcode178
> src/pkg/runtime/mgc0.c:178: if(Debug)
> On 2010/02/03 09:42:08, r wrote:
>> sometimes you say Debug and sometimes you say Debug > 1
> 
> changed former to Debug > 0.
> 
> http://codereview.appspot.com/198085/show

[r2, 2010-02-03 10:13:09]

On 03/02/2010, at 9:11 PM, Chris Wedgwood wrote:

> 
> or similar so i can potentially use the same function for different
> types?

here we go.

-rob

[rsc, 2010-02-03 10:15:10]

i did upload but the links in the email don't take you
to the new copy (grr).
http://codereview.appspot.com/198085/diff2/71:100/108

[rsc, 2010-02-03 10:17:35]

> Should this work as-is?
>
> I just applied this and tried it out in a contrived way and didn't see
> my finalizer do anything after releasing references and calling
> runtime.GC().

There's no guarantee that a particular GC will find a particular block.
There are probably lingering references on your stack that are
keeping the block from being collected at all.  It does work:
try test/mallocfin.go.

>  What's more, can't a finalizer be
>
> func myfinalizer(x interface{}) { }
>
> or similar so i can potentially use the same function for different types?

No.  Finalizers are fundamentally tied to cleanup for a specific type.
You're asking for generality that is neither free nor necessary.

Russ

[cw, 2010-02-03 10:21:37]

On Wed, Feb 03, 2010 at 02:17:30AM -0800, Russ Cox wrote:

> There's no guarantee that a particular GC will find a particular
> block.

true

> There are probably lingering references on your stack that are
> keeping the block from being collected at all.

i put a recursive call in to try and mess that up (fib 10 fwiw)

> ??It does work:
> try test/mallocfin.go.

--- cd ../test
fail: ./mallocfin.go
35,39d34
< =========== ./mallocfin.go
< throw: finalizer table inconsistent
<
< panic PC=xxx
<
0 known bugs; 0 unexpected bugs; test output differs

[r, 2010-02-03 10:22:52]

http://codereview.appspot.com/198085/diff/100/108
File src/pkg/runtime/extern.go (right):

http://codereview.appspot.com/198085/diff/100/108#newcode107
src/pkg/runtime/extern.go:107: // to be unreachable will free x.
x is already unreachable. i'm being pedantic but this must be clear.  maybe
that determines x to be unreachable again?

http://codereview.appspot.com/198085/diff/100/108#newcode125
src/pkg/runtime/extern.go:125: // The finalizer for x runs at some time after x
becomes unreachable.
Some arbitrary time after x becomes unreachable, the finalizer for x is
scheduled to be run.

http://codereview.appspot.com/198085/diff/100/108#newcode126
src/pkg/runtime/extern.go:126: // There is no guarantee that finalizers will run
before a program exits,
there's really no guarantee they run at all.

http://codereview.appspot.com/198085/diff/100/108#newcode129
src/pkg/runtime/extern.go:129: // For example, os.File objects register a
finalizer that closes the
mark this with a TODO please. you might not get to it before someone believes
the comment.

http://codereview.appspot.com/198085/diff/100/108#newcode131
src/pkg/runtime/extern.go:131: // to depend on a finalizer to flush an in-memory
I/O buffer like a
s/like/inside/

[rsc, 2010-02-03 18:13:26]

PTAL

[cw, 2010-02-03 18:29:45]

On Wed, Feb 03, 2010 at 06:13:26PM +0000, rsc@golang.org wrote:

> PTAL

i saw something odd, you can see it if you tweak N mallocfin to say 10
and also instrument the finalizer functions:

  cw@lysdexia:~/wk/go$ touch mallocfin.go && make mallocfin && ./mallocfin
  6g mallocfin.go
  6l -o mallocfin mallocfin.6
  rm mallocfin.6
  finalA
  finalA
  finalA
  finalA
  finalA
  finalA
  finalA
  finalB
  finalB
  finalB
  finalA
  finalB
  finalA
  not enough finalizing:8/10
  panic PC=0x7ff592ea8f68
  finalB
  finalB
  finalB
  finalB
  main.main+0x1be /home/cw/wk/go/mallocfin.go:61
	  main.main()
  mainstart+0xf /home/cw/wk/go/go.hg/src/pkg/runtime/amd64/asm.s:54
	  mainstart()
  goexit /home/cw/wk/go/go.hg/src/pkg/runtime/proc.c:140
	  goexit()

[rsc, 2010-02-03 18:37:51]

> i saw something odd, you can see it if you tweak N mallocfin to say 10
> and also instrument the finalizer functions:

that's normal (and why it does more than 10 iterations).
you're going to have a couple of memory locations on the
stack that still hold references to the most recent few blocks.

russ

[cw, 2010-02-03 21:04:39]

On Wed, Feb 03, 2010 at 10:37:47AM -0800, Russ Cox wrote:

> that's normal (and why it does more than 10 iterations).  you're
> going to have a couple of memory locations on the stack that still
> hold references to the most recent few blocks.

long term (perhaps when there is a different GC) can we expect all
pending finalizers will get run in the normal application termination
path?

[r, 2010-02-03 21:11:53]

http://codereview.appspot.com/198085/diff/149/1182
File src/pkg/runtime/extern.go (right):

http://codereview.appspot.com/198085/diff/149/1182#newcode115
src/pkg/runtime/extern.go:115: // of x's type and returns no arguments.  If
either of these is not
why can it not return something?  it seems the obvious first example would be
SetFinalizer(file, &File.Close)

[cw, 2010-02-03 21:28:02]

http://codereview.appspot.com/198085/diff/149/1182
File src/pkg/runtime/extern.go (right):

http://codereview.appspot.com/198085/diff/149/1182#newcode135
src/pkg/runtime/extern.go:135: // bufio.Writer, because the buffer would not be
flushed at program exit.
This seems like an odd example.

On termination you don't usually need to close files, but flushing unwritten
data is very useful.

So if you were to use a finalizer for some high-level file object, you really
want the behavior that's not guaranteed.

[rsc, 2010-02-03 22:03:05]

> long term (perhaps when there is a different GC) can we expect all
> pending finalizers will get run in the normal application termination
> path?

no.  exiting is exiting.

> This seems like an odd example.
>
> On termination you don't usually need to close files, but flushing
> unwritten data is very useful.
>
> So if you were to use a finalizer for some high-level file object, you
> really want the behavior that's not guaranteed.

hence the comment.

that's not what finalizers are for.  they are not destructors.

see http://www.hpl.hp.com/techreports/2002/HPL-2002-335.html

russ

[rsc, 2010-02-03 22:05:10]

> http://codereview.appspot.com/198085/diff/149/1182#newcode115
> src/pkg/runtime/extern.go:115: // of x's type and returns no arguments.
> If either of these is not
> why can it not return something?  it seems the obvious first example
> would be
> SetFinalizer(file, &File.Close)

i'm happy to remove that restriction in a separate CL.
added a TODO.

[r, 2010-02-03 22:27:08]

LGTM

[rsc, 2010-02-04 00:31:37]

*** Submitted as http://code.google.com/p/go/source/detail?r=82048d222b00 ***

finalizers; merge package malloc into package runtime

R=r, cw
CC=golang-dev
http://codereview.appspot.com/198085