Thankyou for your reply - my responses are embedded below..
On Friday, May 24, 2019 at 1:12:43 AM UTC-4, Jan Mercl wrote:
>
> On Fri, May 24, 2019 at 3:05 AM <zod...@gmail.com <javascript:>> wrote:
> >
> > Unfortunately, after fitting the code with KeepAlive() calls (some
> probably superfluous), the issues and their frequency have not notably
> changed. It did allow me to remove the hack I had put in LockST() described
> in my original note though. I've even gone as far as commenting out the
> finalizers, which because of the flavor of the interface being used,
> probably won't leave much if any C memory stranded but is not the way we
> would want it in production. However it is fine for a test. Even with the
> finalizers out of the way, the issues remain.
> >
> > Is there something else I can try to isolate this (msan and race have
> not been fruitful)? Note our C code has some memory sanity options in it
> too - we have a storage manager in it that on request will over-allocate
> all requests placing the requested storage in the middle, then back-filling
> all but the requested storage with 0xdeadbeef values with the entire
> storage chain having their backfills verified on every storage invocation.
> That too is running clean. Also, the first thing our C code does when it
> gets control is take out a lock because our C code is not (yet)
> multi-threaded so that part is single-threaded.
>
> I took a look at the code. Some things that stand out immediately:
>
> - Dozen of comments in Go code using "Golang" as the name of the language.
>
[SEstes] I had been told by a Go consultant to refer to it as Golang to
reduce confusion (much like how this forum is named). If that's in error I
apologize and will change the references. But for the record, there are 13
references to "golang" in the main directory, not dozens..
>
> - Yoda with happy is
>
> func (buft *BufferT) Free() {
> if nil == buft {
> return
> }
> if true == buft.ownsBuff {
> buft.cbuft.Free()
> }
> buft.cbuft = nil
> }
>
> (
> https://gitlab.com/YottaDB/Lang/YDBGo/blob/4a8c7ab467677c664e1a52ac2b87a9a537cbb605/buffer_t.go#L138)
>
>
>
> - It seems that in more than one place the code violates the rules
> about Go pointers crossing the CGo barrier, for example at
>
> https://gitlab.com/YottaDB/Lang/YDBGo/blob/4a8c7ab467677c664e1a52ac2b87a9a537cbb605/buffer_t.go#L39
[SEstes] Since line 39 is a comment line in the header of a section, I'm
not sure what code you are referring to. But the *only* place that we
knowingly pass a pointer to golang storage into C is in the the key_t.go
routines NodeNextST() and NodePrevST() where we pass a pointer to a Go
uint32 into C (but do so as a C.int because that is what C expects). All
other places where we pass structures or buffers into C we are passing the
address of C.malloc()'d memory which is *anchored* in Go structures. This
is permissible. What is not permissible is passing the address of a
structure (Go or C) that contains pointers to Go storage (at least last I
read - not sure if that has recently changed). We have tried to do what we
needed to do without violating any cgo rules - which has been .. at times
difficult but I think we succeeded.
>
>
> - Exported API that has unsafe.Pointer typed parameters:
>
> https://gitlab.com/YottaDB/Lang/YDBGo/blob/4a8c7ab467677c664e1a52ac2b87a9a537cbb605/buffer_t.go#L39
>
> while not even mentioning what's safe to pass as the unsafe pointer
> argument. Must it be only pointer to C allocated memory or can it be a
> pointer to anything managed by the Go runtime?
>
[SEstes] Admittedly this BufferTFromPtr routine could be documented better.
While the existing comments and the name do mostly hint at its usage, I
will update this to be more descriptive. The routine takes an unsafe
pointer because it is driven by transaction callback routine so the overall
topology is Go -> C -> Go but the C side of this interface has a generic
parameter so it is passed as a void * in C which of course becomes an
unsafe pointer in Go. From discussing with the group, the fact that this
routine is exported is an artifact of an earlier version so it won't be
exported in the next update.
>
> I looked at the code only for a few minutes, but the impression I got
> is that this is not at all about "Aggressive Golang Garbage Collection
> Issues When Using cgo". I think it instead about not
> following/ignoring the documented rules for Go/CGo interaction as well
> as not following/ignoring the documented rules about safe/supported
> uses of unsafe.Pointers.
>
[SEstes] The original problem which was described as being in LockST() was
indeed due to aggressive GC and was cured by Ian's suggestion to use
KeepAlive(). I thought the problems had a single origin but was evidently
wrong. Something else is going on which has yet to be understood.
[SEstes] As for cgo rules, I assure you I have read and put into practice
every documented rule for cgo/Go interaction as well as the uses of unsafe
pointers as best I am able. There are no "known" violations. There are
constraints on what i'm able to do when wrapping existing (released and
largely immutable) interfaces. Obviously if something was just flat not
possible, we'd create a new call to do what we want but I'm of the opinion
that this code follows the rules though since it is interfacing with C and
doesn't support some of the things that C does (array references and
variadic plists for example), we have to get a little off the beaten path
for workarounds. If there is some way the code does NOT follow the rules, I
really would like to know exactly where and how it could work instead if
possible. We do not intend for production code to violate rules. That's
just a recipe for trouble.
[SEstes] Just to make clear that one doesn't need to look at the entire
wrapper, I have a test that is executing a single API call (DeleteE() from
easy_api.go) over and over against an empty database (so it always gets a
not-found error). In 20 minutes of runtime (done as 20 consecutive 1 minute
runs), something around half of those runs will die with either the Go heap
issue or the sweep increased allocation issue. And they generally fail in
the first or last 5 seconds of the run - but of course not always.. Here is
what the test routine looks like (released under AGPL):
//////////////////////////////////////////////////////////////////
// //
// Copyright (c) 2018-2019 YottaDB LLC and/or its subsidiaries. //
// All rights reserved. //
// //
// This source code contains the intellectual property //
// of its copyright holder(s), and is made available //
// under a license. If you do not know the terms of //
// the license, please stop and do not read further. //
// //
//////////////////////////////////////////////////////////////////
package main
/* Run for a specified duration calling random features of the YottaDB Go
Easy API.
We fork off N threads, where has a chance of:
- Getting a global/local
- Setting a global/local
- Killing a global/local
- Data'ing a global/local
- Walking through a global/local
- Walking through a subscript
- Incrementing a global/local
- Settings locks on a global/local
- Starting a TP transaction
- Ending a TP transaction
- Creating routines in a TP transaction
The goal is to run for some time with no panics
*/
import (
"fmt"
"lang.yottadb.com/go/yottadb"
"math/rand"
"sync"
"time"
)
const DRIVER_THREADS = 10
const MAX_DEPTH = 10
const NEST_RATE = 0.20
const TEST_TIMEOUT = 60 // test timeout in seconds for the driver
threads to stop
func Ok() {
// Noop that means everything is OK
}
type testSettings struct {
maxDepth int
nestedTpRate float64
}
/* Generate a variable to be used in runProc()
*/
func genName() []string {
var ret []string
num_subs := rand.Int() % 5
global_id := rand.Int() % 4
switch global_id {
case 0:
ret = append(ret, "^MyGlobal1")
case 1:
ret = append(ret, "^MyGlobal2")
case 2:
ret = append(ret, "MyLocal1xx")
case 3:
ret = append(ret, "MyLocal2xx")
}
for i := 0; i < num_subs; i++ {
ret = append(ret, fmt.Sprintf("sub%d", i))
}
return ret
}
/* randomly attempts various EasyAPI functions with a generated variable
* see header comment for details
*/
func runProc(tptoken uint64, errstr *yottadb.BufferT, settings
testSettings, curDepth int) int32 {
t := genName()
varname := t[0]
subs := t[1:]
delType := rand.Float64()
var err error
if delType < 0.5 {
err = yottadb.DeleteE(tptoken, errstr, yottadb.YDB_DEL_TREE,
varname, subs)
} else {
err = yottadb.DeleteE(tptoken, errstr, yottadb.YDB_DEL_NODE,
varname, subs)
}
// There are some error codes we accept; anything other than that,
raise an error
if err != nil {
panic(err)
}
return 0
}
func main() {
var wg sync.WaitGroup
var doneMutex sync.Mutex
defer yottadb.Exit()
tptoken := yottadb.NOTTP
done := false
go func() { //wait for test timeout then set done to true
time.Sleep(TEST_TIMEOUT * time.Second)
doneMutex.Lock()
done = true
doneMutex.Unlock()
}()
for i := 0; i < DRIVER_THREADS; i++ {
wg.Add(1)
go func() {
for { //loop until test timeout then break
doneMutex.Lock()
d := done
doneMutex.Unlock()
if d {
break
}
runProc(tptoken, nil, testSettings{
MAX_DEPTH,
NEST_RATE,
}, 0)
}
wg.Done()
}()
}
wg.Wait() //wait for existing routines to end
}
[SEstes] A lot of this can be ignored as it is cut down from a test that
did a lot of random API usages. Pretty much any one of API interfaces will
do but just this one causes all of these errors in spades. DeleteE() is in
easy_api.go and invokes a few methods to build up the key, then drives the
C routine - over and over so only uses 5-10% of this wrapper. No variadic
plist is involved, no callbacks, just a simple call. And this spectacularly
fails.
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to golang-nuts+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/golang-nuts/bec74386-28c5-452f-bdc7-f5cbd4ab2423%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
