code review 80960047: runtime: initialize complete Hiter during mapiterinit

runtime: initialize complete Hiter during mapiterinit

The garbage collector will scan these pointers,
so make sure they are initialized.

[rsc, 2014-03-27 00:23:52]

Hello khr (cc: golang-codereviews@googlegroups.com),

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

[bradfitz, 2014-03-27 00:24:59]

LGTM



On Wed, Mar 26, 2014 at 5:23 PM, <rsc@golang.org> wrote:

> Reviewers: khr,
>
> Message:
> Hello khr (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> runtime: initialize complete Hiter during mapiterinit
>
> The garbage collector will scan these pointers,
> so make sure they are initialized.
>
> Please review this at https://codereview.appspot.com/80960047/
>
> Affected files (+8, -0 lines):
>   M src/pkg/runtime/hashmap.goc
>
>
> Index: src/pkg/runtime/hashmap.goc
> ===================================================================
> --- a/src/pkg/runtime/hashmap.goc
> +++ b/src/pkg/runtime/hashmap.goc
> @@ -1004,6 +1004,14 @@
>
>  #pragma textflag NOSPLIT
>  func mapiterinit(t *MapType, h *Hmap, it *Hiter) {
> +       // Clear pointer fields so garbage collector does not complain.
> +       it->key = nil;
> +       it->value = nil;
> +       it->t = nil;
> +       it->h = nil;
> +       it->buckets = nil;
> +       it->bptr = nil;
> +
>         if(h == nil || h->count == 0) {
>                 it->key = nil;
>                 return;
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[khr1, 2014-03-27 00:25:41]

LGTM.


On Wed, Mar 26, 2014 at 5:24 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> LGTM
>
>
>
> On Wed, Mar 26, 2014 at 5:23 PM, <rsc@golang.org> wrote:
>
>> Reviewers: khr,
>>
>> Message:
>> Hello khr (cc: golang-codereviews@googlegroups.com),
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go/
>>
>>
>> Description:
>> runtime: initialize complete Hiter during mapiterinit
>>
>> The garbage collector will scan these pointers,
>> so make sure they are initialized.
>>
>> Please review this at https://codereview.appspot.com/80960047/
>>
>> Affected files (+8, -0 lines):
>>   M src/pkg/runtime/hashmap.goc
>>
>>
>> Index: src/pkg/runtime/hashmap.goc
>> ===================================================================
>> --- a/src/pkg/runtime/hashmap.goc
>> +++ b/src/pkg/runtime/hashmap.goc
>> @@ -1004,6 +1004,14 @@
>>
>>  #pragma textflag NOSPLIT
>>  func mapiterinit(t *MapType, h *Hmap, it *Hiter) {
>> +       // Clear pointer fields so garbage collector does not complain.
>> +       it->key = nil;
>> +       it->value = nil;
>> +       it->t = nil;
>> +       it->h = nil;
>> +       it->buckets = nil;
>> +       it->bptr = nil;
>> +
>>         if(h == nil || h->count == 0) {
>>                 it->key = nil;
>>                 return;
>>
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "golang-codereviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-codereviews+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.
>>
>
>

[khr1, 2014-03-27 00:28:10]

Another option would be to inline hash_iter_init into mapiterinit.  There's
no reason they need to be two separate functions.


On Wed, Mar 26, 2014 at 5:25 PM, Keith Randall <khr@google.com> wrote:

> LGTM.
>
>
> On Wed, Mar 26, 2014 at 5:24 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:
>
>> LGTM
>>
>>
>>
>> On Wed, Mar 26, 2014 at 5:23 PM, <rsc@golang.org> wrote:
>>
>>> Reviewers: khr,
>>>
>>> Message:
>>> Hello khr (cc: golang-codereviews@googlegroups.com),
>>>
>>> I'd like you to review this change to
>>> https://code.google.com/p/go/
>>>
>>>
>>> Description:
>>> runtime: initialize complete Hiter during mapiterinit
>>>
>>> The garbage collector will scan these pointers,
>>> so make sure they are initialized.
>>>
>>> Please review this at https://codereview.appspot.com/80960047/
>>>
>>> Affected files (+8, -0 lines):
>>>   M src/pkg/runtime/hashmap.goc
>>>
>>>
>>> Index: src/pkg/runtime/hashmap.goc
>>> ===================================================================
>>> --- a/src/pkg/runtime/hashmap.goc
>>> +++ b/src/pkg/runtime/hashmap.goc
>>> @@ -1004,6 +1004,14 @@
>>>
>>>  #pragma textflag NOSPLIT
>>>  func mapiterinit(t *MapType, h *Hmap, it *Hiter) {
>>> +       // Clear pointer fields so garbage collector does not complain.
>>> +       it->key = nil;
>>> +       it->value = nil;
>>> +       it->t = nil;
>>> +       it->h = nil;
>>> +       it->buckets = nil;
>>> +       it->bptr = nil;
>>> +
>>>         if(h == nil || h->count == 0) {
>>>                 it->key = nil;
>>>                 return;
>>>
>>>
>>>
>>> --
>>> You received this message because you are subscribed to the Google
>>> Groups "golang-codereviews" group.
>>> To unsubscribe from this group and stop receiving emails from it, send
>>> an email to golang-codereviews+unsubscribe@googlegroups.com.
>>> For more options, visit https://groups.google.com/d/optout.
>>>
>>
>>
>

[rsc, 2014-03-27 00:32:29]

On Wed, Mar 26, 2014 at 8:28 PM, Keith Randall <khr@google.com> wrote:

> Another option would be to inline hash_iter_init into mapiterinit.
>  There's no reason they need to be two separate functions.
>

there's not an atomicity problem here. the Hiter will not be scanned for
pointers until mapiterinit returns (it's not recorded as live in the
calling stack frame until then), so the inlining doesn't matter too much.
these assignments help in the h == nil shortcut and also address the fact
that mapiterinit does not actually set all these fields.

russ

[rsc, 2014-03-27 01:52:32]

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

runtime: initialize complete Hiter during mapiterinit

The garbage collector will scan these pointers,
so make sure they are initialized.

LGTM=bradfitz, khr
R=khr, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/80960047

[gobot, 2014-03-27 02:27:40]

This CL appears to have broken the darwin-amd64-race-cheney builder.
See http://build.golang.org/log/6be44973a76f83d6d255f09d81c6c1b088d8febb