-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
Improve BOM processing performance #218
Conversation
Note: This PR should be squash-merged so it's easier to revert if we have to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great!
@VithikaS Could you please re-approve, given everything still looks OK? Had to resolve some merge conflicts by rebasing. |
src/main/java/org/dependencytrack/persistence/ComponentQueryManager.java
Show resolved
Hide resolved
src/main/java/org/dependencytrack/tasks/BomUploadProcessingTask.java
Outdated
Show resolved
Hide resolved
Signed-off-by: nscuro <nscuro@protonmail.com>
…ty` queries See DependencyTrack/dependency-track#2540 This also cleans the query from containing weird statements like `(cpe != null && cpe == null)` in case a component does not have a CPE. Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
Instead of using individual bulk UPDATE queries, use setters on persistent components instead. This way we can again make use of batched flushing. Signed-off-by: nscuro <nscuro@protonmail.com>
Also decompose large processing method into multiple smaller ones, and re-implement notifications. Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
Also add more documentation Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
... via new transient field. Required for compatibility with #217 Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
The optimal value could depend on how beefy the database server is, and how much memory is available to the API server. Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
To address DependencyTrack/dependency-track#2519 (comment). Also add regression test for this specific issue. Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
…ersistent object state Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
As of DependencyTrack/hyades-apiserver#218, the project's PURL is ingested from the uploaded BOM. Signed-off-by: nscuro <nscuro@protonmail.com>
Description
This refactors
BomUploadProcessingTask
to improve its efficiency and consistency.Notable changes:
ComponentIdentity
before interacting with the databaseflush
ed to the database over the course of the transactionAddressed Issue
Closes DependencyTrack/hyades#635
Additional Details
But does it perform better? Uh, yeah.
I compared this PR with the current
snapshot
build of hyades-apiserver, using the Hyadesdocker-compose.yml
. Other Hyades services besides the API server have been turned off for this test.I uploaded the same 1000 BOMs to each version. The high-level results are as follows:
snapshot
The API server version built from this PR took just about 4min for the entire operation, whereas the current
snapshot
took 15min.System / JVM Statistics
Overall resource footprint is down. Less CPU and Heap usage.
As the Postgres container is running on the same machine, the System CPU usage graph being lower for this PR is a good sign as well. Less stress is being put on the database server.
This PR
snapshot
Database / ORM Statistics
Because all operations of BOM processing now run in a single transaction, the total number of transaction is noticeably down. Also, only the transactional connection pool is utilized. It can be observed that only at the very beginning there are a few connections in pending state.
In contrast, in the
snapshot
version, both connection pools are heavily utilized, and there's a constantly high number of pending connections on the non-transactional pool. The number of transactions is also insanely high.This PR
snapshot
BOM Processing Duration
The average duration of BOM processing stays constantly in the single-digit second range for this PR, whereas for
snapshot
it even reaches up to almost 2min.This PR
snapshot
Kafka Records Produced
Another nice indicator that things have improved: The number of records produced to Kafka per second have noticeably increased. This is just a side effect of BOM processing completing faster, but still nice to observe.
This PR
snapshot
Internal Event System
The laptop I am testing on has 16 CPU cores. DT will thus use a worker thread pool of up to 64 threads. Worker pool threads are responsible for processing BOMs, thus their usage is a good indicator of how performant the processing is.
With this PR, the number of workers used just barely reaches 20 shorty. Also, there were no Events Queued the entire time, indicating that BOMs were processed almost as soon as they were uploaded.
With
snapshot
, the pool was entirely saturated the whole time. And still events were queueing up, eventually reaching 250 yet-to-be-processed events.This PR
snapshot
Checklist
This PR fixes a defect, and I have provided tests to verify that the fix is effectiveThis PR introduces changes to the database model, and I have added corresponding update logicThis PR introduces new or alters existing behavior, and I have updated the documentation accordingly