xoofx / zio

A cross-platform abstract/virtual filesystem framework with many built-ins filesystems for .NET
BSD 2-Clause "Simplified" License
827 stars 61 forks source link

UPath performance improvements #77

Closed bollhals closed 1 year ago

bollhals commented 1 year ago

In my project, we're using your great library.

Lately we've been analyzing our performance, and your library came up a few times. This collection of fixes were mostly some low hanging fruits to improve things here and there a bit. I'll comment on them for better understanding.

There are a few other places that I can improve on, but that's for later if wanted.

xoofx commented 1 year ago

Thanks, I'm ok with the changes. Looks like some tests are failing, if you could have a look.

bollhals commented 1 year ago

I removed the explicit null assert, as if it is null, it can never be absolute. This is the cause of the failing tests, let me know if I should adjust the tests to ArgumentException instead of ArgumentNullException, or whether to put this back.

From here: https://github.com/xoofx/zio/pull/77#discussion_r1348451555

Can you let me know what you prefer?

bollhals commented 1 year ago

Reminder: Which way shall I adjust it to fix the tests?

xoofx commented 1 year ago

I'm on a business trip and pretty busy, will come back hopefully next week, my apologies.

xoofx commented 1 year ago

The CI is failing because dotnet-releaser is requiring net7.0, so I bumped it on main, you can merge main back.

xoofx commented 1 year ago

I had to make further change to main (drop some old targets, but that's ok)

bollhals commented 1 year ago

rebased and solved the failing tests, should be ready.

coveralls commented 1 year ago

Coverage Status

coverage: 90.435% (+0.001%) from 90.434% when pulling 76aae5c90ffb2e49cd204a726de50a10cfc47c80 on bollhals:perf_1 into 99c16ec893e292a698327983c2ad5ba734db25e5 on xoofx:main.

xoofx commented 1 year ago

Thanks for the contribution, my apologies that it took some time and ping/pong to review it. 🙂