Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove "github.com/jinzhu/copier" dependecy #952

Open
Luap99 opened this issue Mar 8, 2022 · 10 comments
Open

Remove "github.com/jinzhu/copier" dependecy #952

Luap99 opened this issue Mar 8, 2022 · 10 comments

Comments

@Luap99
Copy link
Member

Luap99 commented Mar 8, 2022

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 (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?

@Luap99
Copy link
Member Author

Luap99 commented Mar 8, 2022

@vrothberg PTAL

@rhatdan
Copy link
Member

rhatdan commented Mar 8, 2022

@mtrmac @mheon @wdyt?

@vrothberg
Copy link
Member

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.

@Luap99
Copy link
Member Author

Luap99 commented Mar 9, 2022

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.

@mtrmac
Copy link
Contributor

mtrmac commented Mar 10, 2022

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.

@mheon
Copy link
Member

mheon commented Mar 10, 2022

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.

@Luap99
Copy link
Member Author

Luap99 commented Mar 10, 2022

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.

@vrothberg
Copy link
Member

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 containers/podman#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.

@Luap99
Copy link
Member Author

Luap99 commented Mar 11, 2022

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.

@vrothberg
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants