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

Rewrite PRSelf when loading a dependency package #3406

Merged
merged 1 commit into from
Nov 11, 2019
Merged

Conversation

hurryabit
Copy link
Contributor

@hurryabit hurryabit commented Nov 9, 2019

Currently, we first load the package as is and rewrite the PRSelf
references in a second step. This PR moves the rewriting directly
in the loading step.

When buidling simple project that has our favourite large project as a
dependency, this decreased

  • total allocations from 63GB to 57GB
  • run time from 34.0s to 31.5s

Pull Request Checklist

  • Read and understand the contribution guidelines
  • Include appropriate tests
  • Set a descriptive title and thorough description
  • Add a reference to the issue this PR will solve, if appropriate
  • Add a line to the release notes, if appropriate
  • Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.


This change is Reviewable

@hurryabit hurryabit requested review from cocreature and a user November 9, 2019 17:03
Copy link
Contributor

@cocreature cocreature left a comment

Choose a reason for hiding this comment

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

Very nice, thanks a lot!

@hurryabit hurryabit force-pushed the rewrite-prself branch 3 times, most recently from 3a8ce45 to 0b46b64 Compare November 9, 2019 22:20
Copy link

@ghost ghost 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, thanks!

@@ -554,7 +554,8 @@ createProjectPackageDb opts thisSdkVer deps0 dataDeps = do
forM allDalfs $ \(name, dalf) -> do
(pkgId, package) <-
either (fail . DA.Pretty.renderPretty) pure $
Archive.decodeArchive dalf
-- FIXME(MH): This keeps the old behaviour but seems wrong to me.
Copy link

Choose a reason for hiding this comment

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

No need to rewrite self references here, the dalf won't be linked or typechecked and for code generation it's easier if you just have the PRSelf references and translate those to the unit it.

When buidling simple project that has our favourite large project as a
dependency, this decreased
- total allocations from 63GB to 57GB
- run time from 34.0s to 31.5s
@cocreature
Copy link
Contributor

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

@mergify mergify bot merged commit 6fe3df5 into master Nov 11, 2019
@mergify mergify bot deleted the rewrite-prself branch November 11, 2019 08:52
bame-da pushed a commit that referenced this pull request Nov 19, 2019
When buidling simple project that has our favourite large project as a
dependency, this decreased
- total allocations from 63GB to 57GB
- run time from 34.0s to 31.5s
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.

2 participants