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

Add benchmarks for scanning files #36078

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

rdner
Copy link
Member

@rdner rdner commented Jul 17, 2023

What does this PR do?

So, we can compare how much slower the fingerprinting mode against using device and inode in the scanner.

Why is it important?

Our customers would want to know the performance impact.

Benchmark Results

Baseline: main branch before the scanner revamp and the fingerprint mode was introduced

Git HEAD at a755bbc

Average ~33194198,2 ns/op

BenchmarkGetFiles
BenchmarkGetFiles-10    	      31	  33275968 ns/op
BenchmarkGetFiles-10    	      31	  32648550 ns/op
BenchmarkGetFiles-10    	      31	  32753038 ns/op
BenchmarkGetFiles-10    	      30	  33802444 ns/op
BenchmarkGetFiles-10    	      34	  33490991 ns/op

After the scanner revamp and prospector.scanner.fingerprint.enabled: false

Git HEAD at 061cb88

Average ~5398583 ns/op

BenchmarkGetFiles
BenchmarkGetFiles-10                   	     206	   5424156 ns/op
BenchmarkGetFiles-10                   	     210	   5317834 ns/op
BenchmarkGetFiles-10                   	     210	   5396120 ns/op
BenchmarkGetFiles-10                   	     201	   5423895 ns/op
BenchmarkGetFiles-10                   	     205	   5430910 ns/op

The scanner optimisations made in #35734 led to ~84% performance boost in normal mode (device+inode, not fingerprint).

prospector.scanner.fingerprint.enabled: true, prospector.scanner.fingerprint.length: 1024

The results are taken with this change applied #36073

Git HEAD at 061cb88

Average ~22251610,4 ns/op

BenchmarkGetFilesWithFingerprint
BenchmarkGetFilesWithFingerprint-10    	      52	  22186490 ns/op
BenchmarkGetFilesWithFingerprint-10    	      52	  22609885 ns/op
BenchmarkGetFilesWithFingerprint-10    	      52	  22630851 ns/op
BenchmarkGetFilesWithFingerprint-10    	      52	  21972390 ns/op
BenchmarkGetFilesWithFingerprint-10    	      55	  21858436 ns/op

Using fingerprint mode with length 1024 is ~76% slower than the default device+inode mode in the NEW scanner, however it's still faster than the default device+inode in the old scanner (baseline).

prospector.scanner.fingerprint.enabled: true, prospector.scanner.fingerprint.length: 512

BenchmarkGetFilesWithFingerprint
BenchmarkGetFilesWithFingerprint-10    	      52	  21308312 ns/op
BenchmarkGetFilesWithFingerprint-10    	      52	  21821144 ns/op
BenchmarkGetFilesWithFingerprint-10    	      42	  24927905 ns/op
BenchmarkGetFilesWithFingerprint-10    	      54	  22013189 ns/op
BenchmarkGetFilesWithFingerprint-10    	      55	  20895236 ns/op

Using fingerprint mode with a shorter length 512 does not significantly affect the performance

CPU Profile

As you can see from this CPU profile collected for the prospector.scanner.fingerprint.enabled: true, prospector.scanner.fingerprint.length: 1024 case the main contributor – 94,01% is the syscall to either open or close the file. So, optimisations on hashing would not make much of a difference.

Screenshot 2023-07-17 at 12 10 19

Full report:
profile001.pdf

Conclusion

Screenshot 2023-07-17 at 12 48 27

Even activating the fingerprint mode in the new FileScanner it's working faster than the old FileScanner before the optimisations introduced in #35734

Related issues

So, we can compare how much slower the fingerprinting mode against using
`device` and `inode` in the scanner.
@rdner rdner added Filebeat Filebeat Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team backport-7.17 Automated backport to the 7.17 branch with mergify labels Jul 17, 2023
@rdner rdner self-assigned this Jul 17, 2023
@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jul 17, 2023
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-07-17T09:34:47.199+0000

  • Duration: 70 min 13 sec

Test stats 🧪

Test Results
Failed 0
Passed 8046
Skipped 757
Total 8803

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@rdner rdner mentioned this pull request Jul 17, 2023
9 tasks
@rdner rdner marked this pull request as ready for review July 17, 2023 12:31
@rdner rdner requested a review from a team as a code owner July 17, 2023 12:31
@rdner rdner requested review from belimawr and faec July 17, 2023 12:31
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@rdner rdner enabled auto-merge (squash) July 17, 2023 13:38
@rdner rdner requested a review from leehinman July 17, 2023 16:43
@shmsr
Copy link
Member

shmsr commented Jul 17, 2023

Just a suggestion, you can use https://pkg.go.dev/golang.org/x/perf/cmd/benchstat to compare two or more benchmarks.

Copy link
Contributor

@leehinman leehinman left a comment

Choose a reason for hiding this comment

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

LGTM

One possible improvement, might want to use https://pkg.go.dev/testing#B.ResetTimer right before the for loop that does GetFiles, that would remove the time it takes to make the files from the results.

@rdner rdner merged commit b481143 into elastic:main Jul 17, 2023
7 checks passed
mergify bot pushed a commit that referenced this pull request Jul 17, 2023
So, we can compare how much slower the fingerprinting mode against using
`device` and `inode` in the scanner.

(cherry picked from commit b481143)
@rdner rdner deleted the add-file-scanner-benchmarks branch July 17, 2023 19:32
rdner added a commit that referenced this pull request Jul 18, 2023
So, we can compare how much slower the fingerprinting mode against using
`device` and `inode` in the scanner.

(cherry picked from commit b481143)

Co-authored-by: Denis <denis.rechkunov@elastic.co>
Scholar-Li pushed a commit to Scholar-Li/beats that referenced this pull request Feb 5, 2024
So, we can compare how much slower the fingerprinting mode against using
`device` and `inode` in the scanner.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-7.17 Automated backport to the 7.17 branch with mergify Filebeat Filebeat Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants