code review 4813043: runtime: fix memclr and impove memcopy.

runtime: fix memclr and impove memcopy.

[zhai, 2011-07-21 12:26:18]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2011-07-22 05:44:53]

Have you seen a bug that this fixes?

[zhai, 2011-07-22 08:20:41]

On Fri, Jul 22, 2011 at 1:44 PM, <r@golang.org> wrote:

> Have you seen a bug that this fixes?

Please look at the memclr. if argument count == 1, it clear 8  bytes on
amd64, or 4 bytes on 386.
Not sure it's intend to do so.

[r2, 2011-07-22 09:49:24]

On 22/07/2011, at 6:20 PM, zhai wrote:

> On Fri, Jul 22, 2011 at 1:44 PM, <r@golang.org> wrote:
> Have you seen a bug that this fixes?
> Please look at the memclr. if argument count == 1, it clear 8  bytes on amd64,
or 4 bytes on 386.
> Not sure it's intend to do so.

If you're not sure, why do you want to fix it?

-rob

[r2, 2011-07-22 10:13:25]

Let me put it another way. Do you have a test that fails before this fix and
succeeds after it?  Semantic changes to core libraries are not lightly made.

-rob

[dsymonds, 2011-07-22 14:20:01]

On Fri, Jul 22, 2011 at 6:20 PM, zhai <qyzhai@gmail.com> wrote:

> Please look at the memclr. if argument count == 1, it clear 8  bytes on
> amd64, or 4 bytes on 386.

In addition to Rob's comments, I think you've got this around the
wrong way. The current amd64 code has
  MOVQ	16(SP), CX // puts the count in CX
  SHRQ	$3, CX // count >> 3
That means that, for count=1, CX will hold zero, so the "REP; STOSQ"
will not write anything. In effect, the existing memclr under-clears
but never over-clears.


Dave.

[zhai, 2011-07-22 16:57:17]

On Fri, Jul 22, 2011 at 6:13 PM, Rob 'Commander' Pike <r@google.com> wrote:

> Let me put it another way. Do you have a test that fails before this fix
> and succeeds after it?  Semantic changes to core libraries are not lightly
> made.


I have not found any tests fails before this "fix", the memset(memclr) is
just a little  not reasonable to me，I found it some days ago, and hesitate a
while before send a changeset.

I hope it will avoid some  bugs in the future.

sorry for my poor english.

[zhai, 2011-07-22 17:50:26]

>
> If you're not sure, why do you want to fix it?
>
> -rob
>
I mean "is it intended to do so?"

[rsc, 2011-07-23 00:57:58]

the existing code overclears.
it clears up to the next word boundary.
but it is only called from places where
that is okay.

this CL replaces a C byte-clearing loop
in a context where that is not okay with a
call to memclr, so it also fixes memclr
not to overclear, or else the switch would
not be okay.

everything about this CL looks good to me.

[zhai, 2011-07-23 03:50:17]

>
>  so it also fixes memclr
> not to overclear, or else the switch would
> not be okay.
>

It still pass all tests without changing the memclr asm code on 386.

[dsymonds, 2011-07-23 03:52:36]

Oh, duh. Somehow I completely missed the
  ADDQ	$7, CX

Yes, the existing one does overclear.

Serves me right for reading assembly code after midnight.


Dave.

[rsc, 2011-07-23 19:45:01]

LGTM

Don't seem to be any more objections, so submitting...

[rsc, 2011-07-23 19:47:04]

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

runtime: replace byte-at-a-time zeroing loop with memclr

R=golang-dev, r, r, dsymonds, rsc
CC=golang-dev
http://codereview.appspot.com/4813043

Committer: Russ Cox <rsc@golang.org>