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.

Sign up for GitHub

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

Jump to bottom

perf(seed): run rebuilds in parallel, add perf logs #4334

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

perf(seed): run rebuilds in parallel, add perf logs #4334

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
5bddaac
perf(seed): run rebuilds in parallel, add perf logs
ThatOneBro Apr 8, 2024
e0f710a
cleanup: remove stray performance.mark
ThatOneBro Apr 8, 2024
f3941d3
feat(server): add both `serial` and `parallel` modes for seed
ThatOneBro Apr 9, 2024
1c424bc
test(seed): split out seed tests, test in parallel to main tests
ThatOneBro Apr 23, 2024
ba9eaa8
cleanup(compose): rm stray service
ThatOneBro Apr 23, 2024
f98f105
cleanup(package.json): rm stray package
ThatOneBro Apr 23, 2024
6ca8cd1
cleanup(tsconfig): add `seed-tests` dir to tsconfig
ThatOneBro Apr 23, 2024
340ee80
fix(test.sh): make parallel tests fail together
ThatOneBro Apr 23, 2024
e5a9888
fix(actions/build): fix `postgres_seed` port
ThatOneBro Apr 23, 2024
fb90a1a
test(actions/build): see if hardcoding port works
ThatOneBro Apr 23, 2024
b4dfc20
cleanup(coverage): `coverage/seed/{parallel,serial}`
ThatOneBro Apr 23, 2024
ec0746b
cleanup(server/test): don't call `docker-compose` in `test` cmd
ThatOneBro Apr 23, 2024
10bc983
test(coverage): add back coverage of `src` to `seed` tests
ThatOneBro Apr 23, 2024
472be29
test(coverage): add exclusions too
ThatOneBro Apr 23, 2024
f99b615
fix(tests/seed): do minimal setup in seed tests to actually test seeding
ThatOneBro Apr 23, 2024
7e18ad8
cleanup(sonar): rm unnecessary `as`s, use `??` operator
ThatOneBro Apr 23, 2024
dfe2307
refactor(seed): always build promise arr first
ThatOneBro Apr 23, 2024
d71b8cd
refactor(seed): move seed tests back into `src`, filter them
ThatOneBro Apr 24, 2024
8369583
docs(server/jest): add comment about weird glob behavior
ThatOneBro Apr 24, 2024
5bdd757
docs(testing): note running `test:seed:parallel` on fresh install
ThatOneBro Apr 24, 2024
c13e908
revert(seed): restore separate rebuild paths
ThatOneBro Apr 24, 2024
743060f
test(seed): make serial seed test run migrations before test
ThatOneBro May 7, 2024
8080800
chore(package-lock): fix needless updates
ThatOneBro May 7, 2024
ed4a256
chore(package-lock): actually fix needless updates
ThatOneBro May 7, 2024
436298b
test(seed): stop needlessly busting cache
ThatOneBro May 8, 2024
8ad3add
test(seed): always bust cache
ThatOneBro May 8, 2024
8ef8b4b
tweak(jest): set the default test timeout lower
ThatOneBro May 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Prev Previous commit
Next Next commit
refactor(seed): always build promise arr first
  • Loading branch information
@ThatOneBro
ThatOneBro committed May 11, 2024
commit dfe2307d771035af02da6e23e26408955004b43b
21 changes: 10 additions & 11 deletions packages/server/src/seeds/searchparameters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,25 +11,24 @@ import { RebuildOptions, buildRebuildOptions } from './common';
* @param options - Optional options for how rebuild should be done.
*/
export async function rebuildR4SearchParameters(options?: Partial<RebuildOptions>): Promise<void> {
const finalOptions = buildRebuildOptions(options);
const rebuildOptions = buildRebuildOptions(options);
const client = getDatabasePool();
await client.query('DELETE FROM "SearchParameter" WHERE "projectId" = $1', [r4ProjectId]);

const systemRepo = getSystemRepo();

if (finalOptions.parallel) {
const promises = [];
for (const filename of SEARCH_PARAMETER_BUNDLE_FILES) {
for (const entry of readJson(filename).entry as BundleEntry[]) {
promises.push(createParameter(systemRepo, entry.resource as SearchParameter));
}
const promises = [];
for (const filename of SEARCH_PARAMETER_BUNDLE_FILES) {
for (const entry of readJson(filename).entry as BundleEntry[]) {
promises.push(createParameter(systemRepo, entry.resource as SearchParameter));
Copy link
Member

Choose a reason for hiding this comment

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

lol, curious how Postgres handles this

Copy link
Member Author

Choose a reason for hiding this comment

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

We're capped at 10 concurrent connections with the pool, I considered trying to up it and see what happens, but seemed to run fine on my local machine as is.

}
}

if (rebuildOptions.parallel) {
await Promise.all(promises);
Copy link
Member

Choose a reason for hiding this comment

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

Consider always building the array, and then using the parallel option in the last step

if (finalOptions.parallel) {
  await Promise.all(promises);
} else {
  for (const promise of promises) {
    await promise;
  }
}

That way we don't need to worry about the logic getting out of sync.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually this won't work. Filling the array in the serial case doesn't actually execute the promises serially, only waits for them serially. We have to create the promise in the loop where it is awaited in order to not unintentionally parallelize them

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if it's worth introducing a new dependency, but p-limit seems like a pretty elegant and simple solution to abstract away the max concurrency that we want to allow and avoid the if/else code paths.

} else {
for (const filename of SEARCH_PARAMETER_BUNDLE_FILES) {
for (const entry of readJson(filename).entry as BundleEntry[]) {
await createParameter(systemRepo, entry.resource as SearchParameter);
}
for (const promise of promises) {
await promise;
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions packages/server/src/seeds/structuredefinitions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ import { RebuildOptions, buildRebuildOptions } from './common';
* @param options - Optional options for how rebuild should be done.
*/
export async function rebuildR4StructureDefinitions(options?: Partial<RebuildOptions>): Promise<void> {
const finalOptions = buildRebuildOptions(options);
const rebuildOptions = buildRebuildOptions(options);
const client = getDatabasePool();
await client.query(`DELETE FROM "StructureDefinition" WHERE "projectId" = $1`, [r4ProjectId]);

const systemRepo = getSystemRepo();
if (finalOptions.parallel) {
if (rebuildOptions.parallel) {
await Promise.all([
createStructureDefinitionsForBundleParallel(systemRepo, readJson('fhir/r4/profiles-resources.json') as Bundle),
createStructureDefinitionsForBundleParallel(systemRepo, readJson('fhir/r4/profiles-medplum.json') as Bundle),
Expand Down
16 changes: 8 additions & 8 deletions packages/server/src/seeds/valuesets.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,20 @@ import { RebuildOptions, buildRebuildOptions } from './common';
* @param options - Optional options for how rebuild should be done.
*/
export async function rebuildR4ValueSets(options?: Partial<RebuildOptions>): Promise<void> {
const finalOptions = buildRebuildOptions(options);
const rebuildOptions = buildRebuildOptions(options);
const systemRepo = getSystemRepo();
const files = ['v2-tables.json', 'v3-codesystems.json', 'valuesets.json', 'valuesets-medplum.json'];
for (const file of files) {
const bundle = readJson('fhir/r4/' + file) as Bundle<CodeSystem | ValueSet>;
if (finalOptions.parallel) {
const promises = [];
for (const entry of bundle.entry as BundleEntry<CodeSystem | ValueSet>[]) {
promises.push(overwriteResource(systemRepo, entry.resource as CodeSystem | ValueSet, finalOptions));
}
const promises = [];
for (const entry of bundle.entry as BundleEntry<CodeSystem | ValueSet>[]) {
promises.push(overwriteResource(systemRepo, entry.resource as CodeSystem | ValueSet, rebuildOptions));
}
if (rebuildOptions.parallel) {
await Promise.all(promises);
} else {
for (const entry of bundle.entry as BundleEntry<CodeSystem | ValueSet>[]) {
await overwriteResource(systemRepo, entry.resource as CodeSystem | ValueSet, finalOptions);
for (const promise of promises) {
await promise;
}
}
}
Expand Down