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

Avoid AliasAnalysis for Hello_World.enso #10996

Merged
merged 6 commits into from
Sep 9, 2024

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Sep 6, 2024

Pull Request Description

Fixes #10843 in an even better way then #10906. At the end of work on #10906 it has been found that AliasAnalysis is still used for Hello_World.enso. Mostly because when loading its .ir cache, we cannot resolve its Standard.Base.IO import as nobody loaded the Standard.Base package into the system yet. This PR fixes the problem by delaying resolution of BindingsMap internals until one of its getters is called.

At the end Hello_World loads its .ir caches properly without invoking any AliasAnalysis functionality as verified by checking the logs.

Checklist

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

  • All code follows the
    Scala,
    Java,
  • Unit tests have been written where possible.
  • Benchmarks are improved - 1st run - results are good

Steps to Reproduce

The ultimate way to verify the behavior is in debugger. Place a breakpoint to Graph.scala:12 - it should get trigger only once when LocalScope.empty is being created. Possibly also for the second time when execution is over and #10842 kicks in (this second case can be disabled by using --repl for example).

To reproduce from CLI run twice (first run generates the .ir cache) and then checks the logs:

enso$ ./built-distribution/enso-engine-*/enso-*/bin/enso --run test/Benchmarks/src/Startup/Hello_World.enso
enso$ ./built-distribution/enso-engine-*/enso-*/bin/enso --run test/Benchmarks/src/Startup/Hello_World.enso --log-level debug | grep Hello

prior to this PR the log contains:

[enso.org.enso.interpreter.runtime.SerializationPool] Unable to load a cache for module [Hello_World].
[enso.org.enso.interpreter.runtime.SerializationPool] Deserializing module Hello_World from IR file: false
[enso.org.enso.compiler.Compiler] Loading module [Hello_World] from source.
[enso.org.enso.compiler.Compiler] Generating code for module [Hello_World].

with this PR the log contains:

[enso.org.enso.compiler.Compiler] Parsing module [Hello_World].
[enso.org.enso.interpreter.runtime.SerializationPool] Restored IR from cache for module [Hello_World] at stage [AFTER_STATIC_PASSES].
[enso.org.enso.interpreter.runtime.SerializationPool] Deserializing module Hello_World from IR file: true
[enso.org.enso.compiler.Compiler] Generating code for module [Hello_World].

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Sep 6, 2024
@JaroslavTulach JaroslavTulach self-assigned this Sep 6, 2024
@JaroslavTulach JaroslavTulach changed the title Avoid AliasAnalisis for Hello_World.enso Avoid AliasAnalysis for Hello_World.enso Sep 6, 2024
@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/DelayBindingsMapToConcrete branch from 1635cd6 to dfe42a8 Compare September 6, 2024 06:46
val packageRepository = compiler.getPackageRepository
this.toConcrete(packageRepository.getModuleMap)
this.pendingRepository = compiler.getPackageRepository
Some(this)
Copy link
Member Author

@JaroslavTulach JaroslavTulach Sep 6, 2024

Choose a reason for hiding this comment

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

This is the core of the change. Delaying toConcrete until all modules are registered to package repository.

It is also potentially dangerous change. Previously, when the restoreFromSerialization failed, it would return None and that would trigger parsing. Right now we optimistically claim everything is all right and then we may get failures later when someone calls for example ResolvedModule.unsafeModule.

It is probably OK (everything is green; but we aren't testing broken scenarios much), but we should get ready for such reports in the future and fix them when they appear.

context.logSerializationManager(
Level.WARNING,
"Deserialization of " + module.getName() + " failed",
ex
Copy link
Member Author

Choose a reason for hiding this comment

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

@hubertp, when this exception is logged, the stack trace isn't printed on the console. What's the recommended way of logging an exception with stacktrace via context.logSerializationManager?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I'm surprised anymore. It looks like the included application.conf does not redefine the layout of logging, meaning it does not print exceptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will file a separate ticket to enable those. Right now the default and only appender is console. We could easily log those to the file but I'm still against printing those in the console.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Sep 6, 2024

There is 5% speedup on Import_World startup benchmark:

Import_World

obviously Hello_World startup shows lower improvements (around 100ms) and empty_startup shows none (as there is no import). Trying yet another run and the results are also good.

@JaroslavTulach
Copy link
Member Author

I don't trust local benchmarking, but per @Akirathan request I did one:

Old Run

Benchmark                     Mode  Cnt     Score      Error  
Startup.empty_startup         avgt    3  2438.773 ± 2213.236  
Startup.hello_world_startup   avgt    3  3550.110 ±   65.246  
Startup.import_world_startup  avgt    3  4118.481 ±  938.980 

New run

Benchmark                     Mode  Cnt     Score      Error 
Startup.empty_startup         avgt    3  2468.019 ± 1609.600
Startup.hello_world_startup   avgt    3  3323.337 ±  811.669
Startup.import_world_startup  avgt    3  4097.680 ±  569.984

The benchmark fluctuates a lot, but there seems to be no slowdown and possibly there is some speedup.

@JaroslavTulach JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Sep 6, 2024
Copy link
Contributor

@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.

Minor Scala style issues

context.logSerializationManager(
Level.WARNING,
"Deserialization of " + module.getName() + " failed",
ex
Copy link
Contributor

Choose a reason for hiding this comment

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

I will file a separate ticket to enable those. Right now the default and only appender is console. We could easily log those to the file but I'm still against printing those in the console.

@enso-bot
Copy link

enso-bot bot commented Sep 7, 2024

Jaroslav Tulach reports a new STANDUP for yesterday (2024-09-06):

Progress: .

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.

LGTM. Minor nitpicks.

@JaroslavTulach JaroslavTulach merged commit 93252ee into develop Sep 9, 2024
41 checks passed
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/DelayBindingsMapToConcrete branch September 9, 2024 12:21
@enso-bot
Copy link

enso-bot bot commented Sep 10, 2024

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.

Speedup EnsoRootNode.buildFrameDescriptor
3 participants