-
Notifications
You must be signed in to change notification settings - Fork 21
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
fs: system: optimize reflink #234
Conversation
adc9a34
to
b7d8a2a
Compare
10000 reflinks now take about 1sec instead of 2.7sec
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #234 +/- ##
==========================================
+ Coverage 64.39% 64.53% +0.13%
==========================================
Files 27 27
Lines 2039 2047 +8
Branches 323 324 +1
==========================================
+ Hits 1313 1321 +8
Misses 666 666
Partials 60 60 ☔ View full report in Codecov by Sentry. |
def _reflink_darwin(src: "AnyFSPath", dst: "AnyFSPath") -> None: | ||
import ctypes | ||
|
||
clonefile = _clonefile() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move argtypes and restype up too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally, my oversight
|
||
|
||
def _reflink_darwin(src: "AnyFSPath", dst: "AnyFSPath") -> None: | ||
import ctypes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider moving this outside.
@@ -36,7 +37,8 @@ def symlink(source: "AnyFSPath", link_name: "AnyFSPath") -> None: | |||
os.symlink(source, link_name) | |||
|
|||
|
|||
def _reflink_darwin(src: "AnyFSPath", dst: "AnyFSPath") -> None: | |||
@functools.lru_cache(maxsize=1) | |||
def _clonefile(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth running a logger.debug
? Is that expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather know that we are falling back there, so yeah. This is ran 1 time worst case, so not much reason to remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that's true.
10000 reflinks now take about 1sec instead of 2.7sec
Related https://github.com/iterative/dql/pull/1007