storj / uplink

Storj network Go library
MIT License
122 stars 17 forks source link

Binary size is too big! #169

Open ncw opened 3 months ago

ncw commented 3 months ago

We use storj.io/uplink and storj.io/uplink/edge in the Storj backend for rclone.

I've just discovered in https://github.com/rclone/rclone/issues/7998 that the storj backend is adding over 6MiB to the rclone binary! That is nearly 10% of the binary size. Most backends weigh in at about 200k - it is only the ones with big external SDKs that exceed 1MB and the storj backend is by far the largest.

I suspect that this package is pulling in more things than it needs to which are making its way into the rclone binary.

I used https://github.com/Zxilly/go-size-analyzer to examine the rclone binary and it seems storj.io/common/pb is the biggest culprit. Maybe this could be split up into sub packages per task?

I note that AWS split their SDK up into sub modules to help with this problem. The code all still lives in the same single github repo, but subpackages have their own go.mod and with the new workspaces feature that makes it manageable to work on.

Any thoughts on how to improve this?

Thanks

egonelbre commented 3 months ago

I definitely feel the pain as well. Without delving into the backstory of things.

We've been working on https://github.com/storj/picobuf in the background to minimize the binary sizes. There are still some features missing that would allow to replace gogo/protobuf (and protobuf) in general. But, the progress got a stuck due to shifting priorities.

Why a full replacement? Because one of the major problems with protobuf is that it relies on reflection and Method.Call, which forces the compiler into conservative mode, which doesn't allow a lot of code to be elided.

Splitting up ended up feeling like a temporary bandaid for the protobuf situation, so we didn't go that route.

The main reason the bloat exists is due to the proto.RegisterType and fileDescriptor stuff. This means that the compiler won't be able to remove the unused types. I'll first review whether I can remove them, because AFAIK, we don't need that part of protobuf.

storj-gerrit[bot] commented 3 months ago

Change pb: remove descriptors and type registration mentions this issue.

ncw commented 3 months ago

@egonelbre thanks for taking a look at this - much appreciated.

Because one of the major problems with protobuf is that it relies on reflection and Method.Call, which forces the compiler into conservative mode, which doesn't allow a lot of code to be elided.

I did not realise this and that probably explains some of the other bloat in the rclone binary from the HDFS backend which also uses protobufs.

egonelbre commented 3 months ago

So, removing the descriptors only ended up reducing things by 300kb.

I also experimented with using picobuf, which reduced things by 2.5MB compared to the baseline.

current.exe         83315712
no-descriptors.exe  82867712
picobuf.exe         80880128
no-storj.exe        76346368

But, I'm still slightly confused why it ends up that large - even with picobuf. I kind of understand why it ends up larger than other backends (mostly because many other use net/http and hence can share the backend), but not to that extent. Some of our monitoring could be the culprit, due to inlining.

egonelbre commented 3 months ago

As far as I can tell, we might be hitting a inliner bug, which causes some code to be inlined, which probably shouldn't be. I'll need to dig into that more.