code review 72520043: runtime: remove atomic CAS loop from marknogc

runtime: remove atomic CAS loop from marknogc
Spans are now private to threads, and the loop
is removed from all other functions.
Remove it from marknogc for consistency.

[dvyukov, 2014-03-07 17:01:19]

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

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

[bradfitz, 2014-03-07 17:05:00]

> Spans not now private to threads

"Spans are now"?




On Fri, Mar 7, 2014 at 9:01 AM, <dvyukov@google.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com (cc: khr@golang.org,
> rsc@golang.org),
>
> I'd like you to review this change to
> https://dvyukov%40google.com@code.google.com/p/go/
>
>
> Description:
> runtime: remove atomic CAS loop from marknogc
> Spans not now private to threads, and the loop
> is removed from all other functions.
> Remove it from marknogc for consistency.
>
> Please review this at https://codereview.appspot.com/72520043/
>
> Affected files (+2, -16 lines):
>   M src/pkg/runtime/mgc0.c
>
>
> Index: src/pkg/runtime/mgc0.c
> ===================================================================
> --- a/src/pkg/runtime/mgc0.c
> +++ b/src/pkg/runtime/mgc0.c
> @@ -2619,26 +2619,12 @@
>  void
>  runtime·marknogc(void *v)
>  {
> -       uintptr *b, obits, bits, off, shift;
> +       uintptr *b, off, shift;
>
>         off = (uintptr*)v - (uintptr*)runtime·mheap.arena_start;  // word
> offset
>         b = (uintptr*)runtime·mheap.arena_start - off/wordsPerBitmapWord
> - 1;
>         shift = off % wordsPerBitmapWord;
> -
> -       for(;;) {
> -               obits = *b;
> -               if((obits>>shift & bitMask) != bitAllocated)
> -                       runtime·throw("bad initial state for marknogc");
> -               bits = (obits & ~(bitAllocated<<shift)) |
> bitBlockBoundary<<shift;
> -               if(runtime·gomaxprocs == 1) {
> -                       *b = bits;
> -                       break;
> -               } else {
> -                       // more than one goroutine is potentially running:
> use atomic op
> -                       if(runtime·casp((void**)b, (void*)obits,
> (void*)bits))
> -                               break;
> -               }
> -       }
> +       *b = (*b & ~(bitAllocated<<shift)) | bitBlockBoundary<<shift;
>  }
>
>  void
>
>
> --
> 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-07 17:18:55]

LGTM.


On Fri, Mar 7, 2014 at 9:04 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> > Spans not now private to threads
>
> "Spans are now"?
>
>
>
>
> On Fri, Mar 7, 2014 at 9:01 AM, <dvyukov@google.com> wrote:
>
>> Reviewers: golang-codereviews,
>>
>> Message:
>> Hello golang-codereviews@googlegroups.com (cc: khr@golang.org,
>> rsc@golang.org),
>>
>> I'd like you to review this change to
>> https://dvyukov%40google.com@code.google.com/p/go/
>>
>>
>> Description:
>> runtime: remove atomic CAS loop from marknogc
>> Spans not now private to threads, and the loop
>> is removed from all other functions.
>> Remove it from marknogc for consistency.
>>
>> Please review this at https://codereview.appspot.com/72520043/
>>
>> Affected files (+2, -16 lines):
>>   M src/pkg/runtime/mgc0.c
>>
>>
>> Index: src/pkg/runtime/mgc0.c
>> ===================================================================
>> --- a/src/pkg/runtime/mgc0.c
>> +++ b/src/pkg/runtime/mgc0.c
>> @@ -2619,26 +2619,12 @@
>>  void
>>  runtime·marknogc(void *v)
>>  {
>> -       uintptr *b, obits, bits, off, shift;
>> +       uintptr *b, off, shift;
>>
>>         off = (uintptr*)v - (uintptr*)runtime·mheap.arena_start;  //
>> word offset
>>         b = (uintptr*)runtime·mheap.arena_start - off/wordsPerBitmapWord
>> - 1;
>>         shift = off % wordsPerBitmapWord;
>> -
>> -       for(;;) {
>> -               obits = *b;
>> -               if((obits>>shift & bitMask) != bitAllocated)
>> -                       runtime·throw("bad initial state for marknogc");
>> -               bits = (obits & ~(bitAllocated<<shift)) |
>> bitBlockBoundary<<shift;
>> -               if(runtime·gomaxprocs == 1) {
>> -                       *b = bits;
>> -                       break;
>> -               } else {
>> -                       // more than one goroutine is potentially
>> running: use atomic op
>> -                       if(runtime·casp((void**)b, (void*)obits,
>> (void*)bits))
>> -                               break;
>> -               }
>> -       }
>> +       *b = (*b & ~(bitAllocated<<shift)) | bitBlockBoundary<<shift;
>>  }
>>
>>  void
>>
>>
>>
>> --
>> 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-07 17:28:39]

LGTM

[dvyukov, 2014-03-11 13:31:40]

On 2014/03/07 17:05:00, bradfitz wrote:
> > Spans not now private to threads
> 
> "Spans are now"?


Done

[dvyukov, 2014-03-11 13:35:53]

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

runtime: remove atomic CAS loop from marknogc
Spans are now private to threads, and the loop
is removed from all other functions.
Remove it from marknogc for consistency.

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

[gobot, 2014-03-11 14:13:00]

This CL appears to have broken the darwin-386-cheney builder.