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

Improve large expansion sets #1324

Merged
merged 6 commits into from
Feb 28, 2024
Merged

Conversation

goetzrrGit
Copy link
Contributor

Description

Part 1 of 2 for #1025

This PR addresses a stability issue where creating large expansion sets caused server crashes due to excessive resource consumption by worker processes. Here's a summary of the improvements:

Problem:

  • Compiling individual authored logic large expansion sets led to high memory usage by worker processes.
  • Queuing multiple compilation jobs per worker exacerbated the issue, contributing to server crashes.
  • Workers had not cap on resources

Solution:

  • Chunking: Implemented chunking to distribute authored logic compilation tasks among a subset of workers, preventing resource overload on individual processes.
  • Caching: Introduced a TS transpilation cache to eliminate repetitive compilations for the same logic units, significantly reducing memory usage and avoiding unnecessary worker load.
  • Exposing worker properties to fine-tune workers per project

By default, I am spinning up 8 workers and giving them 1GB of heap.

Verification

I was able to create an expansion set with 73 authored logic without a crash. Before the fix 20+ would crash the server.

Future work

Implement a single background worker to transpile the authoring logic while the server is idle and cache the results. Help with the huge upfront cost .ex 70+ expansion logic takes about 13 minutes. This is cpu bounded so a beefier computer would help.

@goetzrrGit goetzrrGit added feature A new feature or feature request fix A bug fix sequencing Anything related to the sequencing domain labels Feb 7, 2024
@goetzrrGit goetzrrGit requested a review from a team as a code owner February 7, 2024 00:12
@dyst5422
Copy link
Contributor

dyst5422 commented Feb 7, 2024

I like the solution here with the caching.

I had a similar need recently for throttling the number of inflight promises and came up with this solution which might be inspiration for a less intrusive solution to the batching. Benefit here is that it allows us to limit the number of in process workers, BUT doesn't block on all of them completing. So we can ensure the workers are all always filled.

/** Throttle number of in flight promises. */
export class InFlightPromiseThrottler {
  private inFlightPromises: Promise<unknown>[] = [];
  private promiseLimit: number;

  public constructor(promiseLimit: number) {
    this.promiseLimit = promiseLimit;
  }

  public async run<T>(promiseFactory: () => Promise<T>): Promise<T> {
    while (this.inFlightPromises.length >= this.promiseLimit) {
      await Promise.race(this.inFlightPromises);
    }
    const promise = promiseFactory();
    this.inFlightPromises.push(promise);
    return promise.finally(() => {
      const index = this.inFlightPromises.indexOf(promise);
      if (index !== -1) {
        this.inFlightPromises.splice(index, 1);
      }
    });
  }
}

Used like

// Somewhere in a higher scope
const workerPromiseThrottler = new InFlightPromiseThrottler(8);

// When we actually do the worker run

const result = await workerPromiseThrottler.run(() => {\\ async code calling piscina.run});

@goetzrrGit
Copy link
Contributor Author

I like the solution here with the caching.

I had a similar need recently for throttling the number of inflight promises and came up with this solution which might be inspiration for a less intrusive solution to the batching. Benefit here is that it allows us to limit the number of in process workers, BUT doesn't block on all of them completing. So we can ensure the workers are all always filled.

/** Throttle number of in flight promises. */
export class InFlightPromiseThrottler {
  private inFlightPromises: Promise<unknown>[] = [];
  private promiseLimit: number;

  public constructor(promiseLimit: number) {
    this.promiseLimit = promiseLimit;
  }

  public async run<T>(promiseFactory: () => Promise<T>): Promise<T> {
    while (this.inFlightPromises.length >= this.promiseLimit) {
      await Promise.race(this.inFlightPromises);
    }
    const promise = promiseFactory();
    this.inFlightPromises.push(promise);
    return promise.finally(() => {
      const index = this.inFlightPromises.indexOf(promise);
      if (index !== -1) {
        this.inFlightPromises.splice(index, 1);
      }
    });
  }
}

Used like

// Somewhere in a higher scope
const workerPromiseThrottler = new InFlightPromiseThrottler(8);

// When we actually do the worker run

const result = await workerPromiseThrottler.run(() => {\\ async code calling piscina.run});

Hi Dylan, Thank you for the suggestion!!! Very cool approach and I will give it a try. I hope all is well with you.

@cohansen cohansen self-requested a review February 20, 2024 20:04
@cohansen cohansen removed their assignment Feb 20, 2024
Copy link
Contributor

@cohansen cohansen left a comment

Choose a reason for hiding this comment

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

Looks good to me, I saw your demo as well and it seemed like it was working well. My only question is do we need to enforce that PromiseThrottler is a singleton?

@goetzrrGit
Copy link
Contributor Author

goetzrrGit commented Feb 28, 2024

Looks good to me, I saw your demo as well and it seemed like it was working well. My only question is do we need to enforce that PromiseThrottler is a singleton?

I am using it as a singleton export but there is a ticket I created to use dependency injection so it wont be a singleton in the future.

#1350

* The user can specify the number of workers and how much heap size each worker can have.
Implement worker options to tailor worker behavior to an application's needs, optimizing resource utilization.
Works better than the original chunking I implemented
Introduce a TypeScript transpiling cache to significantly reduce build times for both expansion sets and sequence expansion. This will be an upfront cost.

Implement chunking to prevent overloading the worker queue with excessive jobs. This effectively manages worker resources and prevents potential the consumption of resources and heap size.
@goetzrrGit goetzrrGit merged commit 6deb94f into develop Feb 28, 2024
6 checks passed
@goetzrrGit goetzrrGit deleted the improve-large-expansion-sets branch February 28, 2024 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature or feature request fix A bug fix sequencing Anything related to the sequencing domain
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants