code review 5371098: reflect: avoid allocation during Type.Field

reflect: avoid allocation during Type.Field

benchmark                      old ns/op    new ns/op    delta
BenchmarkCodeEncoder            54459459     53357932   -2.02%
BenchmarkCodeMarshal            56237601     55004241   -2.19%
BenchmarkCodeDecoder           127630262    126423185   -0.95%
BenchmarkCodeUnmarshal         128767843    125555821   -2.49%
BenchmarkCodeUnmarshalReuse    123195734    122623580   -0.46%
BenchmarkUnmarshalString             911          901   -1.10%
BenchmarkUnmarshalFloat64            822          830   +0.97%
BenchmarkUnmarshalInt64              748          748   +0.00%
BenchmarkSkipValue               1938584      1965871   +1.41%

benchmark                       old MB/s     new MB/s  speedup
BenchmarkCodeEncoder               35.63        36.37    1.02x
BenchmarkCodeMarshal               34.50        35.28    1.02x
BenchmarkCodeDecoder               15.20        15.35    1.01x
BenchmarkCodeUnmarshal             15.07        15.46    1.03x
BenchmarkSkipValue                113.75       112.17    0.99x

Fixes issue 2320.

[rsc, 2011-11-15 17:02:12]

Hello iant, r (cc: golang-dev@googlegroups.com),

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

[r, 2011-11-15 17:07:19]

this is above my gross-out threshold. since the measured gain is so modest, i
suggest waiting for a better solution.

[rsc, 2011-11-15 17:13:32]

On Tue, Nov 15, 2011 at 12:07,  <r@golang.org> wrote:
> this is above my gross-out threshold. since the measured gain is so
> modest, i suggest waiting for a better solution.

okay.  the measured gain will get bigger as other
bottlenecks disappear.  i will push this aside for now.

[r2, 2011-11-15 17:17:31]

compromise: why not create seq in go code?

-rob

[rsc, 2011-11-15 17:20:01]

On Tue, Nov 15, 2011 at 12:17, Rob 'Commander' Pike <r@google.com> wrote:
> compromise: why not create seq in go code?

i don't have a way to define read-only data in go code.
i can create read-write data and hope that people won't
edit f.Index, but that's a little fragile.  not unsafe, but fragile.
i'll put it aside and circle back once other bottlenecks are
gone.  ultimately i think we need to have the compiler give
reflect a little more help with things like the accessible
embedded fields.

[r2, 2011-11-15 17:22:02]

On Nov 15, 2011, at 9:19 AM, Russ Cox wrote:

> On Tue, Nov 15, 2011 at 12:17, Rob 'Commander' Pike <r@google.com> wrote:
>> compromise: why not create seq in go code?
> 
> i don't have a way to define read-only data in go code.
> i can create read-write data and hope that people won't
> edit f.Index, but that's a little fragile.  not unsafe, but fragile.
> i'll put it aside and circle back once other bottlenecks are
> gone.  ultimately i think we need to have the compiler give
> reflect a little more help with things like the accessible
> embedded fields.

i understand. it won't be read only but it will be allocated only once. maybe
safety considerations are too important. i'm not sure.

-rob

[rsc, 2013-03-11 20:45:01]

PTAL

I would like a decision about whether we care about this anymore.

Since last we visited this issue, the compiler has added a pragma, making it
possible for us to write the sequence table in C rather than assembly.

However, the garbage collector has also gotten faster. Whereas before the
speedup was about 7%, now the speedup is only about 2%. And the code still has
the drawback of introducing the possibility of an unrecoverably panic. To date I
don't believe we've ever made writable pointers to read-only memory visible in
user code.

I am inclined not to do this, and to close the issue working as intended. 

Thoughts?

[bradfitz, 2013-03-11 20:47:27]

2 cents: I'd still like to see this on principle, to get rid of allocations.

On Mon, Mar 11, 2013 at 1:45 PM, <rsc@golang.org> wrote:

> PTAL
>
> I would like a decision about whether we care about this anymore.
>
> Since last we visited this issue, the compiler has added a pragma,
> making it possible for us to write the sequence table in C rather than
> assembly.
>
> However, the garbage collector has also gotten faster. Whereas before
> the speedup was about 7%, now the speedup is only about 2%. And the code
> still has the drawback of introducing the possibility of an
> unrecoverably panic. To date I don't believe we've ever made writable
> pointers to read-only memory visible in user code.
>
> I am inclined not to do this, and to close the issue working as
> intended.
>
> Thoughts?
>
>
https://codereview.appspot.**com/5371098/<https://codereview.appspot.com/5371...
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[r, 2013-03-11 20:47:29]

I am in favor of an allocation-free solution if available, but this
implementation has bothered me from the beginning. Happy to see it abandoned.

[remyoudompheng, 2013-03-11 20:48:43]

On 2013/03/11 20:45:01, rsc wrote:
> However, the garbage collector has also gotten faster. Whereas before the
> speedup was about 7%, now the speedup is only about 2%. And the code still has
> the drawback of introducing the possibility of an unrecoverably panic. To date
I
> don't believe we've ever made writable pointers to read-only memory visible in
> user code.
> 
> I am inclined not to do this, and to close the issue working as intended. 
> 
> Thoughts?

I think writable poitners to read-only memory are a bad idea. The best idea I
can have is make StructField.Index a slice of a hidden StructField.index [4]int
so that it doesn't escape and cover most cases.

Rémy.

[rsc, 2013-03-11 20:49:47]

On Mon, Mar 11, 2013 at 4:48 PM, <remyoudompheng@gmail.com> wrote:

> On 2013/03/11 20:45:01, rsc wrote:
>
>> However, the garbage collector has also gotten faster. Whereas before
>>
> the
>
>> speedup was about 7%, now the speedup is only about 2%. And the code
>>
> still has
>
>> the drawback of introducing the possibility of an unrecoverably panic.
>>
> To date I
>
>> don't believe we've ever made writable pointers to read-only memory
>>
> visible in
>
>> user code.
>>
>
>  I am inclined not to do this, and to close the issue working as
>>
> intended.
>
>  Thoughts?
>>
>
> I think writable poitners to read-only memory are a bad idea. The best
> idea I can have is make StructField.Index a slice of a hidden
> StructField.index [4]int so that it doesn't escape and cover most cases.
>

This doesn't work: assignment of a StructField value will not update the
slice, and now you have a slice pointing back into the original StructField.

Russ

[bradfitz, 2013-03-11 22:03:14]

Alternate proposal: https://codereview.appspot.com/7730043 (variation on an
earlier attempt)

No read-only memory in this case, but a small race window if somebody does
modify the slice.  I wouldn't be surprised if Dmitry had some crazy fix for
that.

On Mon, Mar 11, 2013 at 1:45 PM, <rsc@golang.org> wrote:

> PTAL
>
> I would like a decision about whether we care about this anymore.
>
> Since last we visited this issue, the compiler has added a pragma,
> making it possible for us to write the sequence table in C rather than
> assembly.
>
> However, the garbage collector has also gotten faster. Whereas before
> the speedup was about 7%, now the speedup is only about 2%. And the code
> still has the drawback of introducing the possibility of an
> unrecoverably panic. To date I don't believe we've ever made writable
> pointers to read-only memory visible in user code.
>
> I am inclined not to do this, and to close the issue working as
> intended.
>
> Thoughts?
>
>
https://codereview.appspot.**com/5371098/<https://codereview.appspot.com/5371...
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[rsc, 2013-03-11 22:03:31]

Let's drop this please.

[rsc, 2013-03-11 22:03:38]

*** Abandoned ***