code review 116390043: runtime: get rid of free

runtime: get rid of free
Several reasons:
1. Significantly simplifies runtime.
2. This code proved to be buggy.
3. Free is incompatible with bump-the-pointer allocation.
4. We want to write runtime in Go, Go does not have free.
5. Too much code to free env strings on startup.

[dvyukov, 2014-07-29 11:17:39]

Hello golang-codereviews@googlegroups.com (cc: bradfitz@golang.org,
khr@golang.org, r@golang.org, rlh@golang.org, rsc@golang.org),

I'd like you to review this change to
https://dvyukov%40google.com@code.google.com/p/go/

[josharian, 2014-07-29 13:32:55]

https://codereview.appspot.com/116390043/diff/20002/src/pkg/runtime/env_posix.c
File src/pkg/runtime/env_posix.c (right):

https://codereview.appspot.com/116390043/diff/20002/src/pkg/runtime/env_posix...
src/pkg/runtime/env_posix.c:55: // so that they are not allocated using tiny
alloc.
Can this comment and the TinySize checks below now be deleted?

[dvyukov, 2014-07-29 13:37:34]

On 2014/07/29 13:32:55, josharian wrote:
>
https://codereview.appspot.com/116390043/diff/20002/src/pkg/runtime/env_posix.c
> File src/pkg/runtime/env_posix.c (right):
> 
>
https://codereview.appspot.com/116390043/diff/20002/src/pkg/runtime/env_posix...
> src/pkg/runtime/env_posix.c:55: // so that they are not allocated using tiny
> alloc.
> Can this comment and the TinySize checks below now be deleted?

Done

Also deleted MSpan.incache

[dvyukov, 2014-07-29 18:09:48]

On 2014/07/29 13:37:34, dvyukov wrote:
> On 2014/07/29 13:32:55, josharian wrote:
> >
>
https://codereview.appspot.com/116390043/diff/20002/src/pkg/runtime/env_posix.c
> > File src/pkg/runtime/env_posix.c (right):
> > 
> >
>
https://codereview.appspot.com/116390043/diff/20002/src/pkg/runtime/env_posix...
> > src/pkg/runtime/env_posix.c:55: // so that they are not allocated using tiny
> > alloc.
> > Can this comment and the TinySize checks below now be deleted?
> 
> Done
> 
> Also deleted MSpan.incache

Also deleted MCentral.nfree

[btracey, 2014-07-30 16:15:01]

The new malloc.go has the comment: "// If the block will be freed with
runtime·free(), typ must be nil.". It seems that something will need to change
(if only the comment) once free goes away.

[khr, 2014-07-30 23:41:07]

LGTM.

https://codereview.appspot.com/116390043/diff/80001/src/pkg/runtime/mcentral.c
File src/pkg/runtime/mcentral.c (left):

https://codereview.appspot.com/116390043/diff/80001/src/pkg/runtime/mcentral....
src/pkg/runtime/mcentral.c:209: if(s->incache)
It would be nice to have some way of recording which spans are owned by an
MCache, just to check invariants like these.  Maybe we should keep incache?

I'm not sure if it's worth it, though.  Thoughts?

[dvyukov, 2014-07-31 08:54:48]

On 2014/07/30 23:41:07, khr wrote:
> LGTM.
> 
> https://codereview.appspot.com/116390043/diff/80001/src/pkg/runtime/mcentral.c
> File src/pkg/runtime/mcentral.c (left):
> 
>
https://codereview.appspot.com/116390043/diff/80001/src/pkg/runtime/mcentral....
> src/pkg/runtime/mcentral.c:209: if(s->incache)
> It would be nice to have some way of recording which spans are owned by an
> MCache, just to check invariants like these.  Maybe we should keep incache?
> 
> I'm not sure if it's worth it, though.  Thoughts?

Restored incache.

[dvyukov, 2014-07-31 08:55:45]

*** Submitted as https://code.google.com/p/go/source/detail?r=6acc2dd545b2 ***

runtime: get rid of free
Several reasons:
1. Significantly simplifies runtime.
2. This code proved to be buggy.
3. Free is incompatible with bump-the-pointer allocation.
4. We want to write runtime in Go, Go does not have free.
5. Too much code to free env strings on startup.

LGTM=khr
R=golang-codereviews, josharian, tracey.brendan, khr
CC=bradfitz, golang-codereviews, r, rlh, rsc
https://codereview.appspot.com/116390043

[gobot, 2014-07-31 09:15:06]

This CL appears to have broken the windows-386 builder.
See http://build.golang.org/log/c29ea5b3878dc72397fd51b5e77c41c37b90b3aa

[dvyukov, 2014-07-31 12:16:10]

mailed https://codereview.appspot.com/120400043
it become outdated when we've removed settype caching
thanks

On Wed, Jul 30, 2014 at 8:14 PM,  <tracey.brendan@gmail.com> wrote:
> The new malloc.go has the comment: "// If the block will be freed with
runtime·free(), typ must be nil.". It seems that something will need to change
(if only the comment) once free goes away.