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

Resolve imports and exports via cached BindingsMap #8160

Merged

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Oct 26, 2023

Pull Request Description

Addresses #6100 a bit. org.enso.compiler.Compiler.runImportsAndExportsResolution used to take up to three seconds as it triggered deserialization of megabytes of IR caches. Now it is faster as it uses library bindings cache. Alas, overall time isn't shortened, as further compiler passes still load (the same amount of) IR caches in.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
  • All code has been tested:
    • Unit tests continue to pass

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Oct 26, 2023
@JaroslavTulach JaroslavTulach self-assigned this Oct 26, 2023
@JaroslavTulach JaroslavTulach changed the title Wip/jtulach/fast exports and imports resolution 6100 Resolve imports and exports via cached BindingsMap Oct 26, 2023
@JaroslavTulach
Copy link
Member Author

old importsAndExportsResolution

The above picture represents the current state. The picture below shows the after applying this PR. The runImportsAndExportsResolution clearly no longer deserializes IR - just it loads library bindings - when they are available.

new importsAndExportsResolution

@JaroslavTulach JaroslavTulach added CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Keep up to date Automatically update this PR to the latest develop. labels Oct 27, 2023
Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

Nice improvement. Approving, although I would appreciate more clarity (more docs).

Copy link
Collaborator

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

I don't understand the reasons for additional ensureParsed calls

current,
{ u =>
u.bindingsMap(concreteBindings)
u.loadedFromCache(true)
Copy link
Collaborator

@hubertp hubertp Oct 27, 2023

Choose a reason for hiding this comment

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

Should we maybe have more than 2 states? It feels like 1) not yet loaded 2) loaded only bindings 3) full IR loaded would be more appropriate

Copy link
Member Author

Choose a reason for hiding this comment

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

There is nobody reading the "three states" yet. Why shall we keep three states in memory when we only read two?

@JaroslavTulach JaroslavTulach removed the CI: Keep up to date Automatically update this PR to the latest develop. label Oct 27, 2023
@JaroslavTulach JaroslavTulach merged commit da21e51 into develop Oct 28, 2023
31 checks passed
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/FastExportsAndImportsResolution_6100 branch October 28, 2023 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants