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

Remove code for pre-installed plugins & properly check if file exists #740

Merged
merged 2 commits into from Jan 27, 2019

Conversation

Projects
4 participants
@Bond-009
Copy link
Member

commented Jan 27, 2019

  • Remove code for pre-installed plugins
  • Check if file exists instead of catching exceptions

Benchmarks show that catching the exceptions is way slower:

           Method |      Mean |     Error |    StdDev | Rank |
----------------- |----------:|----------:|----------:|-----:|
       FileExists |  5.249 us | 0.0417 us | 0.0390 us |    3 |
           FileEx |  4.068 us | 0.0278 us | 0.0260 us |    2 |
 FileDoesntExists |  1.193 us | 0.0046 us | 0.0041 us |    1 |
     FileDoesntEx | 27.005 us | 0.0668 us | 0.0592 us |    4 |
@anthonylavado
Copy link
Member

left a comment

Understood. Approved!

@anthonylavado anthonylavado added this to Reviewer approved in 10.2.0 Release Jan 27, 2019

@nvllsvm nvllsvm merged commit b4893b9 into jellyfin:master Jan 27, 2019

2 checks passed

ci/dockercloud Your tests passed in Docker Cloud
Details
continuous-integration/drone/pr Build is passing
Details

10.2.0 Release automation moved this from Reviewer approved to Done Jan 27, 2019

@Bond-009 Bond-009 deleted the Bond-009:deadcode branch Jan 27, 2019

@JustAMan
Copy link
Member

left a comment

I see this is already merged, but I think those changes are slightly incorrect.
They don't handle some edge cases like file being there the moment you check but removed before you read and so on. Plus did your benchmarks measure also "good" flows (i.e. when file is there), or did they only measure bad cases when file is not found?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.