feature: pool with wildcard #115

evgfedotov · 2023-11-22T05:17:49Z

Some MVP PR to demonstrate #114

vmg · 2023-11-27T10:37:09Z

generator/generator.go


 func (o ObjectSet) String() string {
 	return fmt.Sprintf("%#v", o)
 }

+func (o ObjectSet) Contains(g protogen.GoIdent) bool {
+	for wildcard := range o.wildcards {
+		if Match(wildcard, fmt.Sprintf("%s.%s", string(g.GoImportPath), g.GoName)) {


I think we'd be better set by using the standard library's path.Match, since the GoImportPath here contains slashes, it'd be good if our wildcard function was slash-aware.

Also, why the composition with Sprintf here? Doesn't protogen.GoIdent have a String method? Pretty sure it does.

Agreed, should use path.Match instead.

protogen.GoIdent have String method. But uses quoted GoImportPath part.
(https://github.com/protocolbuffers/protobuf-go/blob/master/compiler/protogen/protogen.go#L1226)

vmg · 2023-11-27T10:37:57Z

generator/generator.go

 func (o ObjectSet) Set(s string) error {
+	if strings.Contains(s, "*") {


This is not great, and won't work once we use path.Match because those wildcards also support ?. I think we should assume that all patterns are wildcards and just attempt matching accordingly.

vmg · 2023-11-27T10:38:55Z

cmd/protoc-gen-go-vtproto/main.go

 	f.Var(&cfg.Poolable, "pool", "use memory pooling for this object")
+	f.Var(&cfg.PoolableExclude, "pool-exclude", "do not use memory pooling for this object")


Is this a feature we really need? I'd rather drop this flag. It should always be able to write a more careful pattern for Poolable to make sure it doesn't match any unnecessary structs.

vmg · 2023-12-04T11:59:38Z

The new matching looks good but I still don't like poolable-exclude. I'd prefer to keep the CLI surface smaller. Thanks!

evgfedotov · 2023-12-13T23:11:46Z

@vmg Hi! Thanks for your comments.

Sorry for the long answer.

Looks like in cases where wee need multiple deep pathes with differend structures like:

pool=github.com/my/awesome/repo/path/to/proto/one/example.Message
pool=github.com/my/awesome/repo/path/two/to/proto/two/example.Message

where I actually need

pool=github.com/my/awesome/repo/*

usage of path.Match wildcards requires following of full directory depth. For example:

pool=github.com/my/awesome/repo/path/*/*/*/*
pool=github.com/my/awesome/repo/path/*/*/*/*/*

I thinking about usage something like https://github.com/bmatcuk/doublestar. Which provides opportunity for represent different path depths with ** wildcard. Then next example would be:

pool=github.com/my/awesome/repo/**

evgfedotov · 2023-12-13T23:18:40Z

And I'm still not sure about removing poolable-exclude because it could give more flexible api. For example, for proto with a large number of messages from whom I want to remove generating pool feature for one of them. That leads to enumerate every message excep one.

feature: pool paths with wildcard

3615d40

vmg requested changes Nov 27, 2023

View reviewed changes

vmg mentioned this pull request Nov 27, 2023

feature: poolable messages paths with wildcard #114

Open

Append path.Match support

b4f3668

evgfedotov mentioned this pull request Dec 13, 2023

bug: IsMap not checked on message field's ReturnToVTpool #123

Closed

vmg closed this pull request by merging all changes into planetscale:main in ec98e72 Jan 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: pool with wildcard #115

feature: pool with wildcard #115

evgfedotov commented Nov 22, 2023

vmg Nov 27, 2023

evgfedotov Nov 27, 2023 •

edited

Loading

vmg Nov 27, 2023

vmg Nov 27, 2023

vmg commented Dec 4, 2023

evgfedotov commented Dec 13, 2023 •

edited

Loading

evgfedotov commented Dec 13, 2023

		func (o ObjectSet) Set(s string) error {
		if strings.Contains(s, "*") {

		f.Var(&cfg.Poolable, "pool", "use memory pooling for this object")
		f.Var(&cfg.PoolableExclude, "pool-exclude", "do not use memory pooling for this object")

feature: pool with wildcard #115

feature: pool with wildcard #115

Conversation

evgfedotov commented Nov 22, 2023

vmg Nov 27, 2023

Choose a reason for hiding this comment

evgfedotov Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

vmg Nov 27, 2023

Choose a reason for hiding this comment

vmg Nov 27, 2023

Choose a reason for hiding this comment

vmg commented Dec 4, 2023

evgfedotov commented Dec 13, 2023 • edited Loading

evgfedotov commented Dec 13, 2023

evgfedotov Nov 27, 2023 •

edited

Loading

evgfedotov commented Dec 13, 2023 •

edited

Loading