Skip to content
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

Speed up resetting procedure #276

Merged
merged 2 commits into from
Dec 10, 2021
Merged

Speed up resetting procedure #276

merged 2 commits into from
Dec 10, 2021

Conversation

mattchrlw
Copy link
Contributor

@mattchrlw mattchrlw commented Dec 9, 2021

When the Reset button is clicked, currently the app must wait for the /state directory to be cleared before proceeding. This can be disruptive for large datasets with gigabytes of files to be deleted. This PR makes it so that the files are first moved out of the state folder (quicker) before being actually deleted in the background, which should be faster.

I tried using rsync but it led to some race condition issues (deleting after dataset and interface are re-initialised). The moving approach is safest.

Benchmark on ~848MB of files from the gk dataset:

  • Old approach: 0.31405 seconds
  • New approach: 0.16623 seconds

Copy link
Contributor

@nicklambourne nicklambourne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not opposed to the move and delete concept if it truly is faster, but the overall (existing) implementation needs to be rethought.

elpis/engines/common/objects/interface.py Outdated Show resolved Hide resolved
@benfoley
Copy link
Contributor

benfoley commented Dec 9, 2021 via email

@mattchrlw
Copy link
Contributor Author

Could use shutil.rmtree on each of the four dirs.

isn’t this the existing approach that we are replacing? I think Nick was referring to using shutil.rmtree on the whole directory, although I may be wrong.

so that it can be removed without getting resource busy error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants