Skip to content

Conversation

geyslan
Copy link
Member

@geyslan geyslan commented Sep 13, 2024

1. Explain what the PR does

af231a6 chore!: rollback proctree to simple LRU

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.

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.
@geyslan
Copy link
Member Author

geyslan commented Sep 13, 2024

@OriGlassman FYI.

@geyslan geyslan merged commit 34be604 into aquasecurity:main Sep 13, 2024
31 checks passed
rscampos pushed a commit to rscampos/tracee that referenced this pull request Oct 10, 2024
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
rscampos pushed a commit to rscampos/tracee that referenced this pull request Oct 10, 2024
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
rscampos pushed a commit that referenced this pull request Oct 11, 2024
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
@geyslan geyslan deleted the revert-proctree-to-simple-lru branch February 19, 2025 20:16
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.

3 participants