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

Remove extra call to newHscEnvEqWithImportPaths #3676

Merged
merged 6 commits into from
Jul 2, 2023

Conversation

nlander
Copy link
Contributor

@nlander nlander commented Jun 27, 2023

Submitting this at the request of @wz1000
Instead of calling newHscEnvEqWithImportPaths again in ghcSessionDepsDefinition, we only generate a new Unique for the HscEnvEq and update the envUnique field.

@nlander nlander requested a review from pepeiborra as a code owner June 27, 2023 12:19
@nlander
Copy link
Contributor Author

nlander commented Jun 27, 2023

We also update the hscEnv field.

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM!

@michaelpj
Copy link
Collaborator

Worth a comment explaining what's going on?

@wz1000
Copy link
Collaborator

wz1000 commented Jun 30, 2023

newHscEnvEqWithImportPaths recreates the ExportsMap for package dependencies on every call, we probably only want to do that once per session instead of once per call to ghcSessionDepsDefinition (which is called for every single file we need to compile).

@michaelpj
Copy link
Collaborator

put it in the code!

@@ -799,7 +799,12 @@ ghcSessionDepsDefinition fullModSummary GhcSessionDepsConfig{..} env file = do
#endif
session' <- liftIO $ mergeEnvs hsc moduleNode inLoadOrder depSessions

Just <$> liftIO (newHscEnvEqWithImportPaths (envImportPaths env) session' [])
-- Here we avoid a call to to `newHscEnvEqWithImportPaths`, which creates a new
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michaelpj Does this sufficiently explain what's going on here? Is there anything we should add?

Copy link
Collaborator

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would say ask yourself:

  1. If I was reading this for the first time, would I know what is going on and why?
  2. Has this captured the knowledge that I gained in order to make the fix?

@nlander nlander force-pushed the remove-extra-hscenveq branch from ff6d39e to 222b050 Compare June 30, 2023 10:55
@@ -799,7 +799,12 @@ ghcSessionDepsDefinition fullModSummary GhcSessionDepsConfig{..} env file = do
#endif
session' <- liftIO $ mergeEnvs hsc moduleNode inLoadOrder depSessions

Just <$> liftIO (newHscEnvEqWithImportPaths (envImportPaths env) session' [])
-- Here we avoid a call to to `newHscEnvEqWithImportPaths`, which creates a new
Copy link
Collaborator

Choose a reason for hiding this comment

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

lgtm

@michaelpj michaelpj added the merge me Label to trigger pull request merge label Jun 30, 2023
@mergify mergify bot merged commit 7e56257 into haskell:master Jul 2, 2023
@fendor fendor mentioned this pull request Aug 8, 2023
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants