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

Check a local attribute for the SMB log switch instead #38819

Merged
merged 2 commits into from
Jun 10, 2021

Conversation

jvillafanez
Copy link
Member

Description

Use a local attribute to decide whether to log something in the SMB connector or not
It also improves the log handling in the subclasses because the inherited methods will use the same flag

Related Issue

Related to https://github.com/owncloud/enterprise/issues/4594

Motivation and Context

Just deciding to log nothing in the connector takes some time. It might be a few microseconds, but the sheer amount of calls that big installations can make might rise this number a lot.
For a particular setup with a folder with 1000 files and another folder inside, there are 73240 calls to the log method. Although each call could take around 10-15 microseconds per call on average, the total amount takes close to 1 second in order to log nothing.

This PR make the logging calls to take nanoseconds if the logging is disabled. For the same amount of data, the waste should be around 10 miliseconds instead of 1 second.

Side note: the request takes around 35 seconds, so it isn't a big gain.

How Has This Been Tested?

Manually tested. Checked with a propfind against the folder with 1000 files using curl, as well as navigating to the folder using the browser. In both cases, a file scan was forced to be triggered with the propfind.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@update-docs
Copy link

update-docs bot commented Jun 8, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@AlexAndBear AlexAndBear self-requested a review June 10, 2021 06:27
Copy link

@AlexAndBear AlexAndBear left a comment

Choose a reason for hiding this comment

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

Sweet, can you also provide a changelog item?
Small (before, after) benchmark numbers would be put the ice on the cake according to the changelog item.

@jvillafanez
Copy link
Member Author

Before patch
Screenshot from 2021-06-10 09-34-24

After patch
Screenshot from 2021-06-10 09-35-43

That's the data I have. So I guess, without the patch, the method takes 0.63% of the time, and with the patch it takes 0.07%.
I don't really want to put numbers because there are always additional factors to take into account, such as network traffic, server load, etc, that could affect the result.

The data refers to the WND app, but the SMB app should have similar results.

Changelog items incoming.

@sonarcloud
Copy link

sonarcloud bot commented Jun 10, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@AlexAndBear AlexAndBear merged commit a0f94c8 into master Jun 10, 2021
@delete-merged-branch delete-merged-branch bot deleted the smb_faster_innactive_log branch June 10, 2021 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants