liurenjie1024 commented on PR #760: URL: https://github.com/apache/iceberg-rust/pull/760#issuecomment-2801161848
> > Hi, I personally feel that the benefits of avoiding the use of `async_trait` at our `FileWrite` level aren't worth it. I'm going to close this PR now. Feel free to start a discussion about your motivation behind this! > > Sorry for leaving these PRs for a while. > > For motivation, it's more like a general rust improvement, to allow improving from dynamic dispatch to static dispatch. For `async_trait`, every call on its async method will create a `BoxedFuture`, which involves a box allocation. The relative cost of this box allocation depends on the cost of the actual work of function body. If we write/read a large chunk in a single call, then the cost can be ignored, but if we only read/write a small slice, this cost will amplify the overall total cost. > > I haven't done a benchmark on iceberg-rust yet. In RisingWave, we used to have a similar PR [risingwavelabs/risingwave#4182](https://github.com/risingwavelabs/risingwave/pull/4182). In RisingWave's LSM SST iterator, we used to have to create a `BoxedFuture` for every row in SST due to the previous use of `async_trait`. After using static-typed future, we've observed at most 20% improvement in the benchmark. I'm also not a big fan of approach used in this pr, it makes things too complicated to maintain. Perf improvements without clarifying experiments setup and execution seems meaningless to me. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
