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

[v0.22.3] chore!: rollback proctree to simple LRU (#4299) #4348

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

rscampos
Copy link
Collaborator

1. Explain what the PR does

d7083d7 chore!: rollback proctree to simple LRU (#4299)

The expirable LRU cache is not intended to be used in the ProcessTree
type since the TTL would impact its correctness on some scenarios.

Despite TTL can be disabled by setting it to 0, it is not a good idea
due to the potential performance impact of its implementation. See the
comparison between the simple LRU and the expirable LRU below:

| Benchmark                              | Exp LRU |  Simple |    %   |
|                                        |  TTL 0  |   LRU   |  Impr. |
|                                        | (ns/op) | (ns/op) |    *   |
|----------------------------------------|---------|---------|--------|
| GetProcessByHash-Concurrency1          | 4.264   | 6.301   | -47.8% |
| GetProcessByHash-Concurrency2          | 14.91   | 19.85   | -33.2% |
| GetProcessByHash-Concurrency4          | 74.03   | 63.22   | 14.6%  |
| GetProcessByHash-Concurrency8          | 247.2   | 174.7   | 29.3%  |
| GetOrCreateProcessByHash-Concurrency1  | 37.02   | 8.370   | 77.4%  |
| GetOrCreateProcessByHash-Concurrency2  | 80.84   | 23.37   | 71.1%  |
| GetOrCreateProcessByHash-Concurrency4  | 181.6   | 75.67   | 58.4%  |
| GetOrCreateProcessByHash-Concurrency8  | 408.3   | 194.5   | 52.4%  |
| GetThreadByHash-Concurrency1           | 38.13   | 7.675   | 79.9%  |
| GetThreadByHash-Concurrency2           | 79.98   | 22.71   | 71.6%  |
| GetThreadByHash-Concurrency4           | 177.1   | 60.76   | 65.7%  |
| GetThreadByHash-Concurrency8           | 403.2   | 190.0   | 52.9%  |
| GetOrCreateThreadByHash-Concurrency1   | 38.16   | 7.996   | 79.0%  |
| GetOrCreateThreadByHash-Concurrency2   | 79.82   | 23.73   | 70.3%  |
| GetOrCreateThreadByHash-Concurrency4   | 177.1   | 67.13   | 62.1%  |
| GetOrCreateThreadByHash-Concurrency8   | 405.1   | 151.3   | 62.7%  |

Therefore, this commit reverts the ProcessTree to use the simple LRU
cache until a better solution is designed.

* Part of the performance improvement is also due to the removal of the
outer lock done by 44e59d3a9731166b5345a3c9c396544810e46a65.

commit: 34be604 (main), cherry-pick

2. Explain how to test it

3. Other comments

The expirable LRU cache is not intended to be used in the ProcessTree
type since the TTL would impact its correctness on some scenarios.

Despite TTL can be disabled by setting it to 0, it is not a good idea
due to the potential performance impact of its implementation. See the
comparison between the simple LRU and the expirable LRU below:

| Benchmark                              | Exp LRU |  Simple |    %   |
|                                        |  TTL 0  |   LRU   |  Impr. |
|                                        | (ns/op) | (ns/op) |    *   |
|----------------------------------------|---------|---------|--------|
| GetProcessByHash-Concurrency1          | 4.264   | 6.301   | -47.8% |
| GetProcessByHash-Concurrency2          | 14.91   | 19.85   | -33.2% |
| GetProcessByHash-Concurrency4          | 74.03   | 63.22   | 14.6%  |
| GetProcessByHash-Concurrency8          | 247.2   | 174.7   | 29.3%  |
| GetOrCreateProcessByHash-Concurrency1  | 37.02   | 8.370   | 77.4%  |
| GetOrCreateProcessByHash-Concurrency2  | 80.84   | 23.37   | 71.1%  |
| GetOrCreateProcessByHash-Concurrency4  | 181.6   | 75.67   | 58.4%  |
| GetOrCreateProcessByHash-Concurrency8  | 408.3   | 194.5   | 52.4%  |
| GetThreadByHash-Concurrency1           | 38.13   | 7.675   | 79.9%  |
| GetThreadByHash-Concurrency2           | 79.98   | 22.71   | 71.6%  |
| GetThreadByHash-Concurrency4           | 177.1   | 60.76   | 65.7%  |
| GetThreadByHash-Concurrency8           | 403.2   | 190.0   | 52.9%  |
| GetOrCreateThreadByHash-Concurrency1   | 38.16   | 7.996   | 79.0%  |
| GetOrCreateThreadByHash-Concurrency2   | 79.82   | 23.73   | 70.3%  |
| GetOrCreateThreadByHash-Concurrency4   | 177.1   | 67.13   | 62.1%  |
| GetOrCreateThreadByHash-Concurrency8   | 405.1   | 151.3   | 62.7%  |

Therefore, this commit reverts the ProcessTree to use the simple LRU
cache until a better solution is designed.

* Part of the performance improvement is also due to the removal of the
outer lock done by 44e59d3.

commit: 34be604 (main), cherry-pick
Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

LGTM

@rscampos rscampos merged commit 7cee1b9 into aquasecurity:v0.22.0 Oct 11, 2024
2 checks passed
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.

2 participants