https://codereview.appspot.com/143150043/diff/40001/src/runtime/malloc.go File src/runtime/malloc.go (right): https://codereview.appspot.com/143150043/diff/40001/src/runtime/malloc.go#newcode123 src/runtime/malloc.go:123: c.local_tinyallocs++ here is a slight potential for overflow if ...
9 years, 7 months ago
(2014-09-16 22:15:07 UTC)
#6
https://codereview.appspot.com/143150043/diff/40001/src/runtime/malloc.go File src/runtime/malloc.go (right): https://codereview.appspot.com/143150043/diff/40001/src/runtime/malloc.go#newcode123 src/runtime/malloc.go:123: c.local_tinyallocs++ On 2014/09/16 22:15:07, dvyukov wrote: > here is ...
9 years, 7 months ago
(2014-09-16 22:56:41 UTC)
#7
https://codereview.appspot.com/143150043/diff/40001/src/runtime/malloc.go
File src/runtime/malloc.go (right):
https://codereview.appspot.com/143150043/diff/40001/src/runtime/malloc.go#new...
src/runtime/malloc.go:123: c.local_tinyallocs++
On 2014/09/16 22:15:07, dvyukov wrote:
> here is a slight potential for overflow
> if you allocate zillions of 1 byte objects, then you need only 256MB to
overflow
> 32-bit uintptr, this can happen w/o triggering GC nor going into mheap for new
> spans (if we obtain partially filled spans form mcentral)
I'm sorry but I don't understand.
If you only have 256 MB then it seems like you can only do 256M 1-byte
tinyallocs with it.
More generally, local_cachealloc is only an intptr and it seems like (excluding
the subtraction during garbage collection) it must be strictly greater than
local_tinyallocs, so a uintptr should be fine.
LGTM https://codereview.appspot.com/143150043/diff/40001/src/runtime/malloc.go File src/runtime/malloc.go (right): https://codereview.appspot.com/143150043/diff/40001/src/runtime/malloc.go#newcode123 src/runtime/malloc.go:123: c.local_tinyallocs++ On 2014/09/16 22:56:41, rsc wrote: > On ...
9 years, 7 months ago
(2014-09-16 23:26:49 UTC)
#8
LGTM
https://codereview.appspot.com/143150043/diff/40001/src/runtime/malloc.go
File src/runtime/malloc.go (right):
https://codereview.appspot.com/143150043/diff/40001/src/runtime/malloc.go#new...
src/runtime/malloc.go:123: c.local_tinyallocs++
On 2014/09/16 22:56:41, rsc wrote:
> On 2014/09/16 22:15:07, dvyukov wrote:
> > here is a slight potential for overflow
> > if you allocate zillions of 1 byte objects, then you need only 256MB to
> overflow
> > 32-bit uintptr, this can happen w/o triggering GC nor going into mheap for
new
> > spans (if we obtain partially filled spans form mcentral)
>
> I'm sorry but I don't understand.
> If you only have 256 MB then it seems like you can only do 256M 1-byte
> tinyallocs with it.
>
> More generally, local_cachealloc is only an intptr and it seems like
(excluding
> the subtraction during garbage collection) it must be strictly greater than
> local_tinyallocs, so a uintptr should be fine.
you are right
ignore this comment
I renamed TinyAllocs to TinyMallocs, but that got me thinking. One thing we could do ...
9 years, 7 months ago
(2014-09-17 00:33:00 UTC)
#9
I renamed TinyAllocs to TinyMallocs, but that got me thinking. One thing we
could do is add the amount for TinyAllocs to both Mallocs and Frees. Then
Mallocs goes up for testing.AllocsPerRun and other users, without needing
to export a new field, and Mallocs - Frees continues to be meaningful.
I'm leaning toward doing that. Thoughts?
Russ
sounds good to me On Tue, Sep 16, 2014 at 5:33 PM, Russ Cox <rsc@golang.org> ...
9 years, 7 months ago
(2014-09-17 00:35:23 UTC)
#10
sounds good to me
On Tue, Sep 16, 2014 at 5:33 PM, Russ Cox <rsc@golang.org> wrote:
> I renamed TinyAllocs to TinyMallocs, but that got me thinking. One thing we
> could do is add the amount for TinyAllocs to both Mallocs and Frees. Then
> Mallocs goes up for testing.AllocsPerRun and other users, without needing to
> export a new field, and Mallocs - Frees continues to be meaningful.
>
> I'm leaning toward doing that. Thoughts?
>
> Russ
>
>
Did this just go full circle? Wasn't that the original idea? On Tue, Sep 16, ...
9 years, 7 months ago
(2014-09-17 02:27:10 UTC)
#11
Did this just go full circle? Wasn't that the original idea?
On Tue, Sep 16, 2014 at 8:35 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> sounds good to me
>
> On Tue, Sep 16, 2014 at 5:33 PM, Russ Cox <rsc@golang.org> wrote:
> > I renamed TinyAllocs to TinyMallocs, but that got me thinking. One thing
> we
> > could do is add the amount for TinyAllocs to both Mallocs and Frees. Then
> > Mallocs goes up for testing.AllocsPerRun and other users, without
> needing to
> > export a new field, and Mallocs - Frees continues to be meaningful.
> >
> > I'm leaning toward doing that. Thoughts?
> >
> > Russ
> >
> >
>
Issue 143150043: code review 143150043: runtime: account for tiny allocs, for testing.AllocsPerRun
(Closed)
Created 9 years, 7 months ago by rsc
Modified 9 years, 7 months ago
Reviewers:
Base URL:
Comments: 4