common icon indicating copy to clipboard operation
common copied to clipboard

Remove "github.com/jinzhu/copier" dependecy

Open Luap99 opened this issue 3 years ago • 10 comments

I am currently trying to shrink the podman binary size. Looking through all dependencies I see that github.com/jinzhu/copier is only used a single time. While this package is only 204 K it imports database/sql which is 1.1 MB in size. By removing this we can save 1.3 MB.

From the Commit (c53283f5fc9a7bdbf727478bc5e6e32ce490524a) messsage which added this dep:

(*Runtime) systemContextCopy() returns a deep copy of the runtime's system context. Golang's shallow copies are very dangerous for long running processes such as Podman's system service. Hence, we need to make sure that base data is not altered over time. That adds another external dependency but I do not see a way around that. Long term, I desire a (*containers/image/types.SystemContext).Copy() function.

So maybe we should just need to add a copy function in c/image?

Luap99 avatar Mar 08 '22 18:03 Luap99

@vrothberg PTAL

Luap99 avatar Mar 08 '22 18:03 Luap99

@mtrmac @mheon @WDYT?

rhatdan avatar Mar 08 '22 19:03 rhatdan

I am currently trying to shrink the podman binary size. Looking through all dependencies I see that github.com/jinzhu/copier is only used a single time.

I don't think that the number of static callers is a good metric to decide which dependencies can easily be pruned. This candidate seems easy to prune though.

While this package is only 204 K it imports database/sql which is 1.1 MB in size. By removing this we can save 1.3 MB.

Great catch!

From the Commit (c53283f) messsage which added this dep:

(*Runtime) systemContextCopy() returns a deep copy of the runtime's system context. Golang's shallow copies are very dangerous for long running processes such as Podman's system service. Hence, we need to make sure that base data is not altered over time. That adds another external dependency but I do not see a way around that. Long term, I desire a (*containers/image/types.SystemContext).Copy() function.

So maybe we should just need to add a copy function in c/image?

I would love to have a function that creates a deep copy of a types.SystemContexct. Looking at it, it seems fairly easy to do "manually". Doing something smart via reflection etc. would certainly be easier to maintain in the future.

vrothberg avatar Mar 09 '22 08:03 vrothberg

I don't think that the number of static callers is a good metric to decide which dependencies can easily be pruned. This candidate seems easy to prune though.

Of course it is not a very good metric. I just followed the import path from database/sql to github.com/jinzhu/copier to github.com/containers/common/libimage. Since it is only needed to copy, I figured that is can be easily replaced by something else.

Luap99 avatar Mar 09 '22 09:03 Luap99

I would love to have a function that creates a deep copy of a types.SystemContexct.

Yes, that would be useful in various places in the containers repos.

Doing something smart via reflection etc. would certainly be easier to maintain in the future.

… and yes, we need to, to an extent, worry about forgetting to update the code, and only implementing a shallow copy. I don’t have a good idea how that could be done.


How about finding some other reflection-based deep-copy Go package, with fewer dependencies? It seems quite possible to me that something like that exists, e.g. random search finds https://github.com/barkimedes/go-deepcopy which at least doesn’t look implausible in a 3-minute look. That would be a very local change in c/common.

mtrmac avatar Mar 10 '22 18:03 mtrmac

May I suggest something similar to Podman's JSONDeepCopy? Given the highly-optimized JSON libraries available, I suspect it's not significantly slower than a reflection-based solution, and it depends only on something we're already using.

I would really like a code generation solution, but the only one I've found is in Kubernetes, and I've yet to figure out how to convince it to work outside their repo.

mheon avatar Mar 10 '22 18:03 mheon

I don't think a code generation solution is good if we try to reduce binary size. K8s is the biggest dependency in podman because it has so much generated code for each struct.

Luap99 avatar Mar 10 '22 19:03 Luap99

May I suggest something similar to Podman's JSONDeepCopy? Given the highly-optimized JSON libraries available, I suspect it's not significantly slower than a reflection-based solution, and it depends only on something we're already using.

I am against using JSONDeepCopy. It is very maintainable but quite expensive (see https://github.com/containers/podman/pull/11781) and hungry for memory. Since libimage is a hot call path, I would aim for something more performant.

I don't think a code generation solution is good if we try to reduce binary size. K8s is the biggest dependency in podman because it has so much generated code for each struct.

I doubt that a generated deep copy for a single struct (i.e., types.SystemContext) would have a measurable impact on the binary size. Certainly, if we'd do that for more structs in such an aggressive way as K8s does, it would accumulate quickly.

I am supportive of exploring the idea of using a generator - it would certainly be the most performant solution. If we can recycle the generator from K8s, even better - less work for us.

vrothberg avatar Mar 11 '22 09:03 vrothberg

I doubt that a generated deep copy for a single struct (i.e., types.SystemContext) would have a measurable impact on the binary size. Certainly, if we'd do that for more structs in such an aggressive way as K8s does, it would accumulate quickly.

Yes this my concern, for a single struct it will be fine.

Luap99 avatar Mar 11 '22 10:03 Luap99

Just had another look at this issue. Given podman is now using database/sql as well, the expected benefits are much less.

I still think it would be nice to have a generator, so I'll keep it open.

vrothberg avatar May 24 '23 13:05 vrothberg