Why did you decide to return a value instead of a pointer?

what do you mean? what that PR is doing is removing pointers in the whole SE (since it was suggested in #1398 and I really do not know why we need pointers in that cases). so you asking why does that PR exist in general?

I understand PR existence, and accepting parameters by value is really good. But what about return values? The common approach we use is returning (*T, error). What are pros to return (T, error)? I mean why don't we return just T which contains error? Maybe I've missed some discussion.

The common approach we use is returning (*T, error).

i saw some examples in the SE where it already was as func(prm Prm) (Res, error). why is it common? to be able to return nil res?

I mean why don't we return just T which contains error

to support common practice in go: start handling results only if err == nil (and know that operation could be finished with error and simplify working with Res struct)

Maybe I've missed some discussion.

no, that was my decision

What are pros to return (T, error)?

If we return a pointer, the struct will definitely be allocated on heap (all these functions are big enough not to be inlined). And in case of T there is no allocation. It is more beneficial than passing parameters by value, actually, because parameters can still be allocated on stack as they do not outlive the called function (in most of our cases).

I mean why don't we return just T which contains error

This is possible, but would make the code non-idiomatic for no real reason IMO. We should still check for the error.

Until the building itself, we do not know exactly how the data will be allocated since this is not visible from the source code. Are there any objective reasons for the change? If it's really a bottleneck, then we need to provide test proof.

We've already discussed this earlier, and previously I voted against return (*T, error) for the same GC optimization and suggested to accept result by pointer as a parameter. This will allow to reuse the same instance between several calls, and most definitely avoid heap allocation. But we've decided with @carpawell @alexvanin to not do it like this. At the same time I don't see returning (T, error) convenient.

I suggest to see what the profiler shows.

1. It is visible from the source if we return value and the function is big enough not to be inlined. The compiler has no option here. Consider this code:

package main

type X struct {
    a int
}

func main() {
    println(getX().a)
}

//go:noinline
func getX() *X {
    return &X{a: 1}
}

go build -gcflags -m ./kek.go
# command-line-arguments
./kek.go:14:12: &X{...} escapes to heap

2. Certainly, this isn't a bottleneck, because we allocate much more inside functions. But the whole point of the epic is to do small improvements here and there to reduce GC pressure.
3. The benchmarks are needed here. Raw parameters can have no effect, but I expect replacing return value to have some benefits in terms of memory. @carpawell Could you write a benchmark for the (simplest) case of metabase.Get?

1. There is no "heap" in terms of Go source code. In your example you make a decision according to the specific compiler. What does it mean "big enough not to be inlined"? How can I get it from the sources?

And again... I am aware and agree that returning a pointer leads rather to worse performance. Therefore, I propose a compromise: allocate a structure to record the results on the calling side and pass a pointer to it in the function.

package main

type X struct {
	a int
}

func main() {
	v := new(X)
	initX(v)

	println(v.a)
}

//go:noinline
func initX(v *X) {
	v.a = 1
}

$ go build -gcflags -m ./main.go 
# command-line-arguments
./main.go:7:6: can inline main
./main.go:15:12: v does not escape
./main.go:8:10: new(X) does not escape

i reorganized that PR: it now only contains *Prm -> Prm changes

Res changes will be added via another PR

1. It is visible from the source if we return value and the function is big enough not to be inlined. The compiler has no option here. Consider this code:

package main

type X struct {
    a int
}

func main() {
    println(getX().a)
}

//go:noinline
func getX() *X {
    return &X{a: 1}
}

go build -gcflags -m ./kek.go
# command-line-arguments
./kek.go:14:12: &X{...} escapes to heap

2. Certainly, this isn't a bottleneck, because we allocate much more inside functions. But the whole point of the epic is to do small improvements here and there to reduce GC pressure.
3. The benchmarks are needed here. Raw parameters can have no effect, but I expect replacing return value to have some benefits in terms of memory. @carpawell Could you write a benchmark for the (simplest) case of metabase.Get?

unrelated to this commit

fixed and checked other places one more time

btw, it is one more argument for changing *Res -> Res: we do not have a uniformity in that cases, and it is not clear what to expect nil or allocated new(Res) as an empty result

it can be beneficial to allocate declare them inside the callback now.

what for?

Because after removal of return value from With* functions, it can be allocated on stack.

And it will remain correct if we decide to parallelize iteration over shards.

Because after removal of return value from With* functions, it can be allocated on stack.

Isn't it still true for a one var declaration before the callback call?

And it will remain correct if we decide to parallelize iteration over shards.

agree here, moved inside the callback

Unrelated to this commit.

unrelated to this commit

unrelated to this commit

unrelated to this commit

unrelated to this commit

unrelated to this commit

same here