Skip to content

Conversation

InsertCreativityHere
Copy link
Member

Our current logic is as follows:
Whenever a user changes a file, we trigger a compilation:

  • Step 1 is converting the root_uri to a root_path
  • Step 2, resolve any user-configured reference directories (relative to root_path)
  • Step 3, construct a new SliceOptions with those references
  • Step 4, perform the compilation
  • Step 5, store the SliceOptions we just created and the CompilationState(Result) returned by slicec in SharedState.

This PR makes the following improvements:

  • Step 1 only needs to happen once. We only set the root_uri at initialization, and the only thing we do with it is convert it into a root_path. Now, we do this conversion once, on server start-up, and store the root_path instead of root_uri.
  • Steps 2 and 3 only need to happen when configuration changes, not every time we re-compile.
    Now, whenever the config changes, we update the references and cache them up-front.
  • We're double-storing the references. Once in SliceConfig, and once in SliceOptions. These are redundant.
    Now we store the SliceOptions directly in SliceConfig. Since the SliceOptions only depends on the config. Nothing else.
    • After this change, SharedState only holds a CompilationState. So there's no reason to have SharedState anymore. We just hold the CompilationState directly.

Comment on lines 128 to +139
let current_files = &self
.shared_state
.compilation_state
Copy link
Member

@externl externl Nov 30, 2023

Choose a reason for hiding this comment

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

I think there's a race condition here. Another thread could perform a compilation or anything else affecting the compilation_state between caching the current files and performing the compilation.

Maybe open an issue unless you want to fix it here since it's somewhat related.

Copy link
Member Author

Choose a reason for hiding this comment

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

@externl, just to be clear, are you talking about this case?:

   THREAD 1                THREAD 2
update_config
       .
       .             trigger_compilation
       .
trigger_compilation

Copy link
Member

@externl externl Nov 30, 2023

Choose a reason for hiding this comment

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

more specifically

   THREAD 1                                    THREAD 2
modify_state|copy_from_state
       .
       .                                trigger_compilation
       .
trigger_compilation

Copy link
Contributor

@ReeceHumphreys ReeceHumphreys left a comment

Choose a reason for hiding this comment

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

lgtm

@InsertCreativityHere InsertCreativityHere force-pushed the cache-reference-directories branch from 611ad4f to 28c67da Compare December 6, 2023 19:12
@InsertCreativityHere InsertCreativityHere merged commit 65b808a into zeroc-ice:main Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants