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

Reuse the same Engine object between ledger resets #466

Merged
merged 2 commits into from
Apr 15, 2019

Conversation

gerolf-da
Copy link
Contributor

@gerolf-da gerolf-da commented Apr 12, 2019

Compared to older versions of the Engine, we do A LOT more validation
now. So when starting with a fresh engine after each reset (via
the ResetService), we also repeat the validation of the loaded packages
again and again. This is VERY expensive, especially for large DAML
packages.

Luckily, the specification of the ResetService states the following:
// Resets the ledger state. Note that loaded DARs won't be removed --
// this only rolls back the ledger to genesis.
This means that we can re-use the same Engine object and benefit from
not having "re-compile" packages via
ConcurrentCompiledPackages#addPackage, more specifically we can avoid
executing this line by having retained all compiled packages in the engine:

Compiler(packages orElse state.packages).compilePackage(pkgId)

Fixes #178

Pull Request Checklist

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.

@gerolf-da gerolf-da added the component/ledger Sandbox and Ledger API label Apr 12, 2019
@gerolf-da gerolf-da force-pushed the reuse-engine-between-resets branch from d40f87a to 57cdb0b Compare April 12, 2019 22:12
Compared to older versions of the Engine, we do A LOT more validation
now. So when starting with a fresh engine after each reset (via
the ResetService), we also repeat the validation of the loaded packages
again and again. This is VERY expensive, especially for large DAML
packages.

Luckily, the specification of the ResetService states the following:
// Resets the ledger state. Note that loaded DARs won't be removed --
// this only rolls back the ledger to genesis.
This means that we can re-use the same Engine object and benefit from
not having "re-compile" packages via
ConcurrentCompiledPackages#addPackage, more specifically we can omit
this line: https://github.com/digital-asset/daml/blob/1f2246c822acd1d44a6b0ed223f0b063f4a4e2cf/daml-lf/engine/src/main/scala/com/digitalasset/daml/lf/engine/ConcurrentCompiledPackages.scala#L56

Fixes #178
@gerolf-da gerolf-da force-pushed the reuse-engine-between-resets branch from 57cdb0b to 999d0fd Compare April 12, 2019 22:34
@jberthold-da
Copy link
Contributor

Looking great!
However, there is a scala test failing (why on Linux only ? Is that test run on MacOS, too?).

Copy link
Contributor

@gabor-aranyossy gabor-aranyossy left a comment

Choose a reason for hiding this comment

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

Nice, thanks @gerolf-da

Copy link
Contributor

@jberthold-da jberthold-da left a comment

Choose a reason for hiding this comment

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

Test speed is restored with this change.

@gerolf-da gerolf-da merged commit 2f3189d into master Apr 15, 2019
@gerolf-da gerolf-da deleted the reuse-engine-between-resets branch April 15, 2019 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/ledger Sandbox and Ledger API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sandbox performance regression in versions 100.11.X
3 participants