Keep validation workers supplied with concepts instead of every 10 - #1
Open
dionmcm wants to merge 1 commit into
Open
Keep validation workers supplied with concepts instead of every 10#1dionmcm wants to merge 1 commit into
dionmcm wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Branch:
perf/rule-executor-rolling-workers· Module:snomed-drools-engineProblem
RuleExecutor.doValidateComponentsbuilt exactlythreads(10) single-conceptCallables, submitted them withinvokeAll, waited for all 10 to finish, cleared the list, and built the next 10.On a full Snapshot that is 72,240 barriers, each running at the speed of its slowest concept. Per-concept cost varies enormously — a concept with 200 descriptions against one with 2 — so nine threads routinely parked while one finished.
Evidence
Sampling the worker threads during a full-Snapshot run, before the change:
Ten threads doing the work of about 2.5. After the change, same workload:
Change
Start
threadslong-lived workers once. Each keeps its ownStatelessKieSessionlist — unchanged ownership, so no new sharing — and claims the next concept from a sharedAtomicIntegeruntil the list is exhausted. Also removes 722,404Callableallocations.Two smaller fixes that belong with it:
exceptionContentsis now a synchronized list. Workers already appended to it concurrently on the old code path; with a plainArrayLista validation error raised by two concepts at once could be lost or corrupt the list.runTasksnow callsFuture.get(), so a worker that dies from an unexpected throwable fails the run instead of silently reducing parallelism. PreviouslyinvokeAll's results were discarded.Test
RuleExecutorTest.testWorkersClaimNextConceptWithoutBatchBarrierbuilds 11 concepts where concept 1 blocks until concept 11 has been visited. Under the old batching, concept 11 is in the second batch of 10 and can never be reached, so the test deadlocks to its timeout and fails. It passes only when a free worker is allowed to run ahead. Verified failing before the change, passing after.Notes for review
Concept ordering within the run is no longer deterministic between threads. It was not deterministic before either — 10 concepts ran concurrently — and findings are per-concept, which the identical-TSV result confirms.
About this series
One of five independent performance changes to the Drools validation path, measured against a real 722,404-concept AU edition Snapshot. Each is a single commit on
master, builds and passes the full suite alone, and leaves the findings byte-identical: 10,949 before and after, with matching report TSVs.Together they take a full-Snapshot validation from 636s to 118s.
This PR and
perf/scale-validation-workerstouch adjacent lines; whichever merges second needs a one-line rebase. There is no logical dependency and the order does not matter.