Skip to content

Move the simulation thread pool from the model to the driver#382

Merged
mattdailis merged 4 commits intodevelopfrom
feature/lift-execution-service
Nov 11, 2022
Merged

Move the simulation thread pool from the model to the driver#382
mattdailis merged 4 commits intodevelopfrom
feature/lift-execution-service

Conversation

@Twisol
Copy link
Copy Markdown
Contributor

@Twisol Twisol commented Oct 17, 2022

  • Tickets addressed: N/A
  • Review: By commit
  • Merge strategy: Merge (no squash)

Description

This PR moves the ExecutionService for thread pooling from the model into the driver -- specifically, the SimulationEngine. Anything in the model that spawns a task now passes a TaskFactory back to the driver, rather than constructing a Task locally. This avoids a great deal of the work previously necessary to make the ExecutionService available wherever it was needed in the model, and generally streamlines interactions between the two sides.

As a result of these changes:

  • RootModel is no longer necessary (as it simply bundle the ExecutionService alongside the mission's own model).
  • The MerlinExtension no longer needs a model provided to it in test cases, so MerlinTextContext is no longer necessary. Also, the type parameters on MerlinExtension have gone away, meaning we can always use @ExtendWith(MerlinExtension.class) rather than the awkward field-based @RegisterExtension. Hurray!
  • We can automatically use Loom's virtual threads if we detect that we're running in a JVM with them enabled. (We don't currently deploy images with --enable-preview present, but now it should be easier than ever to trial simulation on Loom.)

Verification

Build succeeds. I haven't actually tried enabling Loom on this PR because it's kind of a pain to configure Gradle, but if Loom isn't available it will fall back to the existing thread pool anyway. (I did actually try Loom just before JDK 19 was released, and it worked without hiccups. Anyway, if you run with --enable-preview, you're explicitly opting in to potential instability, so...)

Documentation

Probably some of the testing documentation needs to change. I also updated a code snippet for ActivityMapper that referenced RootModel when I was removing it.

Future work

  • Move registration of resources and topics out of the model constructor. This will, as a side-effect, streamline tests even more, because we won't need to pass in a Registrar.
  • Allow models to allocate cells during simulation instead of only at construction time. This would avoid the need for a test constructor entirely. Entirely! (But also, it opens up some interesting patterns for shared local simulation state between tasks that there isn't much reason to forbid.)

@Twisol Twisol requested a review from a team as a code owner October 17, 2022 23:48
@Twisol Twisol requested review from camargo and mattdailis October 17, 2022 23:48
@Twisol Twisol temporarily deployed to e2e-test October 17, 2022 23:48 Inactive
@Twisol Twisol force-pushed the feature/lift-execution-service branch from bb51346 to a0d1bab Compare October 18, 2022 03:42
@Twisol Twisol temporarily deployed to e2e-test October 18, 2022 03:42 Inactive
@mattdailis mattdailis added the breaking change A change that will require updating downstream code label Oct 28, 2022
@camargo camargo added the refactor A code change that neither fixes a bug nor adds a feature label Nov 2, 2022
@mattdailis mattdailis force-pushed the feature/lift-execution-service branch from a0d1bab to 2abfa74 Compare November 5, 2022 03:52
@mattdailis mattdailis temporarily deployed to e2e-test November 5, 2022 03:52 Inactive
@mattdailis mattdailis temporarily deployed to e2e-test November 5, 2022 04:10 Inactive
@mattdailis mattdailis force-pushed the feature/lift-execution-service branch from 00caf77 to 29e7081 Compare November 8, 2022 17:21
@mattdailis mattdailis temporarily deployed to e2e-test November 8, 2022 17:21 Inactive
@mattdailis mattdailis force-pushed the feature/lift-execution-service branch from 29e7081 to 858e565 Compare November 8, 2022 17:21
@mattdailis mattdailis temporarily deployed to e2e-test November 8, 2022 17:22 Inactive
@mattdailis mattdailis force-pushed the feature/lift-execution-service branch from 858e565 to cc67f20 Compare November 8, 2022 19:31
@mattdailis mattdailis temporarily deployed to e2e-test November 8, 2022 19:31 Inactive
@mattdailis mattdailis mentioned this pull request Nov 8, 2022
8 tasks
@mattdailis mattdailis force-pushed the feature/lift-execution-service branch from cc67f20 to 51da7ea Compare November 11, 2022 22:38
@mattdailis mattdailis temporarily deployed to e2e-test November 11, 2022 22:38 Inactive
@mattdailis mattdailis force-pushed the feature/lift-execution-service branch from 51da7ea to 495c98d Compare November 11, 2022 22:39
@mattdailis mattdailis temporarily deployed to e2e-test November 11, 2022 22:39 Inactive
@pcrosemurgy pcrosemurgy self-requested a review November 11, 2022 22:39
@mattdailis mattdailis merged commit dbebc2b into develop Nov 11, 2022
@mattdailis mattdailis deleted the feature/lift-execution-service branch November 11, 2022 23:03
camargo added a commit to NASA-AMMOS/plandev-ui that referenced this pull request Nov 15, 2022
JosephVolosin pushed a commit to NASA-AMMOS/plandev-ui that referenced this pull request Aug 20, 2024
JosephVolosin pushed a commit to NASA-AMMOS/plandev-ui that referenced this pull request Oct 21, 2024
dandelany pushed a commit to NASA-AMMOS/plandev-ui that referenced this pull request Aug 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change A change that will require updating downstream code refactor A code change that neither fixes a bug nor adds a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants