Add a monomorphization cache to the declaration engine. #2637

tritao · 2022-08-29T13:10:21Z

This introduces a cache to the declaration engine that keeps track of functions/structs that have been instantiated from a specific set of type parameters.

The approach here is relatively simple and relies on a linear search. I originally thought about having more maps to speed it up by keying on the type arguments, but since it adds a bit of complexity I opted for this for now. If it proves to be a bottleneck in the future we can always speed it up.

This was prototyped in tritao/declaration-engine-and-collection-context-demo@c3aa5e7, where these APIs where wired to the function application and struct expressions instantiation code.

However given the declaration engine is not hooked up yet here, this PR is just adding the APIs, and more proper testing will be added as we wire things up.

Closes #2636.

sway-core/src/declaration_engine/declaration_engine.rs

emilyaherbert

My recommendation is that we leave this PR as a draft until we have at least one feature already in master in that will allow us to test it (either #2631 or #2635)

tritao · 2022-08-29T16:54:11Z

My recommendation is that we leave this PR as a draft until we have at least one feature already in master in that will allow us to test it (either #2631 or #2635)

I would rather get this now because it introduces new cache-aware APIs, the following PRs for those issues will depend on these APIs anyway and will build on top of this PR. I think your concerns around the stability of this makes sense though, so I've disabled the cache by default and we can enable it by default at a later stage.

emilyaherbert · 2022-08-29T22:45:09Z

so I've disabled the cache by default and we can enable it by default at a later stage

Good plan 👍

sway-core/src/declaration_engine/declaration_id.rs

emilyaherbert

We need to think a little bit longer about how to achieve optimal results with the cache I think, could you iterate on the design another time?

sway-core/src/declaration_engine/declaration_engine.rs

emilyaherbert · 2022-09-01T17:29:35Z

sway-core/src/declaration_engine/declaration_engine.rs

+            .cloned()
+        {
+            let monomorphized_fn_decl = copy.expect_function(&Span::dummy()).unwrap();
+            if &monomorphized_fn_decl.type_parameters == type_arguments {


I think that we may need to think more deeply about the logic of this line. In general, most generic functions are called without type arguments, so most of the time this check will fail, missing valid cache opportunities.

sway-core/src/declaration_engine/declaration_engine.rs

tritao · 2022-09-05T12:05:00Z

sway-core/src/type_system/mod.rs

@@ -7,7 +7,7 @@ mod resolved_type;
 mod trait_constraint;
 mod type_argument;
 mod type_binding;
-mod type_engine;
+pub(crate) mod type_engine;


@emilyaherbert This is needed for the following error:

error[E0603]: module `type_engine` is private --> sway-core/src/declaration_engine/declaration_engine.rs:14:19 | 14 | type_system::{type_engine::monomorphize, EnforceTypeArguments}, | ^^^^^^^^^^^ private module | note: the module `type_engine` is defined here --> sway-core/src/type_system/mod.rs:10:1 | 10 | mod type_engine; | ^^^^^^^^^^^^^^^

Instead of changing the visibility of the module, change the import statement to match this example:

sway/sway-core/src/semantic_analysis/type_check_context.rs

Line 6 in 9076eb2

insert_type, monomorphize, unify_with_self, CopyTypes, EnforceTypeArguments,

This introduces a cache to the declaration engine that keeps track of functions/structs that have been instantiated from a specific set of type parameters. This was originally prototyped in tritao/declaration-engine-and-collection-context-demo@c3aa5e7, where these APIs where wired to the function application and struct expressions instantiation code. However given the declaration engine is not hooked up yet here, this PR is just adding the APIs, and more proper testing will be added as we wire things up. Closes FuelLabs#2636.

tritao · 2022-09-05T12:29:32Z

Updated the PR with a more generic design, threaded some more error results throughout, added enum declarations to DeclarationWrapper as well for enum support.

@emilyaherbert I'm leaving the future improvements (caching generics called without type parameters) to a future issue (opening one), as right now this is blocking 2 other PRs and that can only be tested properly once the declarations are hooked to the engine anyway.

emilyaherbert

Adding the monomorphization cache is not a blocker for adding the declaration engine, and the cache-aware API is similar enough to the non-cache-aware API that switching API functions would only be a small change. Considering we are not able to test the code in the cache until the declaration engine goes in, I recommend that we merge #2631 and #2635 in before we add the cache. Would you close this PR? I recommend keeping this branch alive though so that we can bring it back when we are ready to add the cache again.

sway-core/src/declaration_engine/declaration_engine.rs

emilyaherbert · 2022-09-05T21:20:05Z

sway-core/src/declaration_engine/declaration_engine.rs

+            .iter()
+            .cloned()
+        {
+            if monomorphized_decl.type_parameters() == type_arguments {


This line will cause the cache to fail in some cases (e.g. nested generic function calls).

For example this:

fn foo<T, F>(a: T, b: F) -> F { b } fn bar<T, F>(a: T, b: F) -> F { foo::<T, F>(a, b) }

would generate a false cache hit due to the implementation of PartialEq on TypeInfo::UnknownGeneric:

sway/sway-core/src/type_system/type_info.rs

Line 205 in a989e6c

(Self::UnknownGeneric { name: l }, Self::UnknownGeneric { name: r }) => l == r,

emilyaherbert · 2022-09-05T21:20:31Z

sway-core/src/declaration_engine/declaration_engine.rs

    }

-    fn de_get_monomorphized_struct_copies(
+    pub(crate) fn de_get_or_create_monomorphized_decl<T>(


Remove the pub(crate)

emilyaherbert · 2022-09-05T21:21:47Z

sway-core/src/declaration_engine/declaration_wrapper.rs

@@ -17,6 +17,7 @@ use crate::{
 pub(crate) enum DeclarationWrapper {
    // no-op variant to fulfill the default trait
    Unknown,
+    Enum(TypedEnumDeclaration),


There is already a PR for adding enum support: #2713. Please remove these additions on this PR.

emilyaherbert · 2022-09-05T21:26:06Z

sway-core/src/declaration_engine/declaration_wrapper.rs

+            | DeclarationWrapper::TraitImpl(_)
+            | DeclarationWrapper::Storage(_)
+            | DeclarationWrapper::Unknown => {
+                panic!("declaration type does not support type parameters")


In general, we should not use panic! (unwrap!, etc), unless absolutely necessary. The compiler is designed so that if there is an error when type checking one AST nodes, the other AST nodes are relatively unaffected and can continue to be type checked, providing the best end-user experience. panic! generates an early exit, preventing the compiler from offering this feature to users. This is why we have the heafty CompileResult struct. In this particular case, remove this panic and I recommend that you return an empty vec instead.

emilyaherbert · 2022-09-05T21:27:10Z

sway-core/src/declaration_engine/declaration_wrapper.rs

+    }
+
+    fn to_wrapper(&self) -> DeclarationWrapper {
+        panic!("not expected to be called for DeclarationWrapper")


Remove the panic!

Centril · 2022-09-06T16:16:51Z

sway-core/src/declaration_engine/declaration_engine.rs

+        monomorphized_copies
+            .entry(*original_id)
+            .and_modify(|f| f.push(new_id.clone()))
+            .or_insert_with(|| vec![new_id.clone()]);


Suggests that smallvec might be an improvement here... i.e. there tends to be a single monomorphized copy? Hard to know without a benchmark of course...

sway-core/src/declaration_engine/declaration_engine.rs

tritao · 2022-10-27T11:05:57Z

Closing this for now

tritao self-assigned this Aug 29, 2022

tritao requested a review from emilyaherbert August 29, 2022 13:10

tritao added the compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen label Aug 29, 2022

tritao marked this pull request as ready for review August 29, 2022 13:51

tritao requested a review from a team August 29, 2022 14:36

vaivaswatha previously approved these changes Aug 29, 2022

View reviewed changes

sway-core/src/declaration_engine/declaration_engine.rs Outdated Show resolved Hide resolved

sway-core/src/declaration_engine/declaration_engine.rs Outdated Show resolved Hide resolved

tritao dismissed vaivaswatha’s stale review via 7209d08 August 29, 2022 15:44

tritao force-pushed the de-monomorph-cache branch from 9725feb to 7209d08 Compare August 29, 2022 15:44

vaivaswatha previously approved these changes Aug 29, 2022

View reviewed changes

emilyaherbert suggested changes Aug 29, 2022

View reviewed changes

tritao dismissed vaivaswatha’s stale review via 562fcb9 August 29, 2022 16:50

emilyaherbert requested review from emilyaherbert and a team August 29, 2022 22:44

emilyaherbert approved these changes Aug 29, 2022

View reviewed changes

emilyaherbert suggested changes Aug 29, 2022

View reviewed changes

sway-core/src/declaration_engine/declaration_id.rs Outdated Show resolved Hide resolved

tritao marked this pull request as draft August 31, 2022 11:39

emilyaherbert mentioned this pull request Aug 31, 2022

Make the DeclarationEngine a lazy_static as a semi-temporary bandaid #2675

Merged

tritao force-pushed the de-monomorph-cache branch from 1c4f378 to 248ab50 Compare September 1, 2022 13:02

tritao mentioned this pull request Sep 1, 2022

Add structs to the declaration engine #2688

Merged

tritao marked this pull request as ready for review September 1, 2022 14:15

emilyaherbert requested review from emilyaherbert and vaivaswatha September 1, 2022 17:19

emilyaherbert suggested changes Sep 1, 2022

View reviewed changes

emilyaherbert requested a review from a team September 1, 2022 17:36

tritao force-pushed the de-monomorph-cache branch 2 times, most recently from 2a59131 to e8b9e2a Compare September 5, 2022 12:02

tritao commented Sep 5, 2022

View reviewed changes

tritao force-pushed the de-monomorph-cache branch from e8b9e2a to 44bd2c5 Compare September 5, 2022 12:06

emilyaherbert suggested changes Sep 5, 2022

View reviewed changes

Centril reviewed Sep 6, 2022

View reviewed changes

tritao marked this pull request as draft September 7, 2022 13:25

tritao closed this Oct 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a monomorphization cache to the declaration engine. #2637

Add a monomorphization cache to the declaration engine. #2637

tritao commented Aug 29, 2022

emilyaherbert left a comment

tritao commented Aug 29, 2022

emilyaherbert commented Aug 29, 2022

emilyaherbert left a comment

emilyaherbert Sep 1, 2022

tritao Sep 5, 2022

emilyaherbert Sep 5, 2022

tritao commented Sep 5, 2022

emilyaherbert left a comment

emilyaherbert Sep 5, 2022

emilyaherbert Sep 5, 2022

emilyaherbert Sep 5, 2022

emilyaherbert Sep 5, 2022

emilyaherbert Sep 5, 2022

Centril Sep 6, 2022

tritao commented Oct 27, 2022

Add a monomorphization cache to the declaration engine. #2637

Add a monomorphization cache to the declaration engine. #2637

Conversation

tritao commented Aug 29, 2022

emilyaherbert left a comment

Choose a reason for hiding this comment

tritao commented Aug 29, 2022

emilyaherbert commented Aug 29, 2022

emilyaherbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tritao commented Sep 5, 2022

emilyaherbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tritao commented Oct 27, 2022