Closed Colecf closed 9 months ago
I think you are right. I also kind of suspect a BufWriter
constructed even without the capacity set ahead of time might be enough. I think the main difference might be is that my WriteBuf
allocates wholly on the stack, which means the Writer
impl is free to allocate them in an inner function. Meanwhile a BufWriter
is implemented using a Vec
so it needs to allocate its buffer on the heap, which means it might be worth stashing as a member on the Writer
struct for reuse.
In general if you had a big .n2_db
from an Android build I'd love to see it, we could use it to benchmark db reading/writing in isolation from the rest of n2. I have no real picture right now if db read/write is even a bottleneck.
Maybe something like https://github.com/evmar/n2/compare/bufwrite?expand=1
Oh I also just made https://github.com/evmar/n2/compare/main...Colecf:n2:fix_writebuf_panic?expand=1, but either solution works.
Here's the n2_db that triggers the issue (after fixing the issue), but it's just the n2_db from the bootstrap phase of the build, I haven't gotten the main build working yet. The main build's will be way bigger. This is also from AOSP, the google-internal build graph is bigger.
When building android, we get this panic:
Android splits its build into 2 ninja invocations, a "bootstrap" invocation, and a main invocation. The bootstrap invocation generates the main build.ninja file, and it has a depfile that discovers all the Android.bp files. The depfile has 11842 entries, so it causes the WriteBuf to overflow.
I see there's a comment on WriteBuf about how we could use a
BufWriter
instead, but it might be slightly less efficient. I'm thinking we should probably benchmark if that's the case.BufWriter
does have awith_capacity
constructor, so if we used the same buffer size I'm not sure how it would be any different fromWriteBuf
.