code review 40370061: runtime: do not zero terminate strings

runtime: do not zero terminate strings
On top of "tiny allocator" (cl/38750047), reduces number of allocs by 1% on json.
No code must rely on zero termination. So will also make debugging simpler,
by uncovering issues earlier.

json-1
allocated                 7949686      7915766      -0.43%
allocs                      93778        92790      -1.05%
time                    100957795     97250949      -3.67%
rest of the metrics are too noisy.

[dvyukov, 2013-12-31 12:39:01]

Hello golang-codereviews@googlegroups.com,

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

[r, 2013-12-31 19:20:00]

LGTM

https://codereview.appspot.com/40370061/diff/40001/src/pkg/runtime/string.goc
File src/pkg/runtime/string.goc (right):

https://codereview.appspot.com/40370061/diff/40001/src/pkg/runtime/string.goc...
src/pkg/runtime/string.goc:49: // leave room for NUL for C runtime (e.g.,
callers of getenv)
delete comment.

[dvyukov, 2014-01-17 07:38:14]

This may break existing SWIG binding, because they may check str.p[str.n]
looking for zero terminator, which is not valid after this change...

[bradfitz, 2014-01-17 17:38:44]

So updated SWIG code mapping from gostring->char* would always have to
alloc + copy, right?

Maybe this isn't worth it.



On Thu, Jan 16, 2014 at 11:38 PM, <dvyukov@google.com> wrote:

> This may break existing SWIG binding, because they may check
> str.p[str.n] looking for zero terminator, which is not valid after this
> change...
>
>
> https://codereview.appspot.com/40370061/
>
> --
> 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/groups/opt_out.
>

[dvyukov, 2014-01-17 18:10:53]

On 2014/01/17 17:38:44, bradfitz wrote:
> So updated SWIG code mapping from gostring->char* would always have to
> alloc + copy, right?
> 
> Maybe this isn't worth it.

On http/template I see:
BenchmarkEscapedExecute         5373         4774  -11.15%

and with https://codereview.appspot.com/38750047/
it will be even better, because more small strings will be combined.

[bradfitz, 2014-01-17 18:13:00]

On Fri, Jan 17, 2014 at 10:10 AM, <dvyukov@google.com> wrote:

> On 2014/01/17 17:38:44, bradfitz wrote:
>
>> So updated SWIG code mapping from gostring->char* would always have to
>> alloc + copy, right?
>>
>
>  Maybe this isn't worth it.
>>
>
> On http/template I see:
> BenchmarkEscapedExecute         5373         4774  -11.15%
>
> and with https://codereview.appspot.com/38750047/
> it will be even better, because more small strings will be combined.
>

But what about the SWIG cost?

What percentage of current gostring->char* conversions in SWIG go through
the allocation-free path? Does it matter?

[iant, 2014-01-17 18:46:11]

On Fri, Jan 17, 2014 at 10:12 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>
> What percentage of current gostring->char* conversions in SWIG go through
> the allocation-free path? Does it matter?

In actual practice the interesting path is C++ string <-> Go string.

Turning a Go string into a C char* is error-prone no matter what.  Not
all C char* values are nul-terminated strings.

Ian

[bradfitz, 2014-01-17 18:49:30]

On Fri, Jan 17, 2014 at 10:46 AM, Ian Lance Taylor <iant@golang.org> wrote:

> On Fri, Jan 17, 2014 at 10:12 AM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> >
> > What percentage of current gostring->char* conversions in SWIG go through
> > the allocation-free path? Does it matter?
>
> In actual practice the interesting path is C++ string <-> Go string.
>
> Turning a Go string into a C char* is error-prone no matter what.  Not
> all C char* values are nul-terminated strings.
>

I agree. I just don't know how often people use each method.

I guess improving this at the cost of making C char* slower to SWIG is
fine--- it'll encourage them not to do that. And it'll cause more crashes
in non-SWIG cgo/unsafe code improperly assuming null-terminated. That in
itself seems like a win, even without any memory/performance improvement.

[dvyukov, 2014-01-18 15:32:32]

On Fri, Jan 17, 2014 at 10:49 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> On Fri, Jan 17, 2014 at 10:46 AM, Ian Lance Taylor <iant@golang.org> wrote:
>>
>> On Fri, Jan 17, 2014 at 10:12 AM, Brad Fitzpatrick <bradfitz@golang.org>
>> wrote:
>> >
>> > What percentage of current gostring->char* conversions in SWIG go
>> > through
>> > the allocation-free path? Does it matter?
>>
>> In actual practice the interesting path is C++ string <-> Go string.
>>
>> Turning a Go string into a C char* is error-prone no matter what.  Not
>> all C char* values are nul-terminated strings.
>
>
> I agree. I just don't know how often people use each method.
>
> I guess improving this at the cost of making C char* slower to SWIG is
> fine--- it'll encourage them not to do that. And it'll cause more crashes in
> non-SWIG cgo/unsafe code improperly assuming null-terminated. That in itself
> seems like a win, even without any memory/performance improvement.
>

Ian, good point about std::string.
Another point about char* is that it does not support 0x00 in the
middle of string, while Go strings do.
And, yes, I was debugging a bug in our initial swig binding which were
expecting all Go strings (including "substrings") to be
zero-terminated. I did not expect the bug to be there, because it was
kind of "working" for a long time. "Fail early and loudly" would be
useful.

[dvyukov, 2014-01-24 18:29:09]

https://codereview.appspot.com/40370061/diff/40001/src/pkg/runtime/string.goc
File src/pkg/runtime/string.goc (right):

https://codereview.appspot.com/40370061/diff/40001/src/pkg/runtime/string.goc...
src/pkg/runtime/string.goc:49: // leave room for NUL for C runtime (e.g.,
callers of getenv)
On 2013/12/31 19:20:01, r wrote:
> delete comment.

Done.

[dvyukov, 2014-01-24 18:29:25]

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

runtime: do not zero terminate strings
On top of "tiny allocator" (cl/38750047), reduces number of allocs by 1% on
json.
No code must rely on zero termination. So will also make debugging simpler,
by uncovering issues earlier.

json-1
allocated                 7949686      7915766      -0.43%
allocs                      93778        92790      -1.05%
time                    100957795     97250949      -3.67%
rest of the metrics are too noisy.

LGTM=r
R=golang-codereviews, r, bradfitz, iant
CC=golang-codereviews
https://codereview.appspot.com/40370061