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

Instantiates generics in the module that uses it #22513

Merged
merged 19 commits into from
Sep 9, 2023

Conversation

jmgomez
Copy link
Collaborator

@jmgomez jmgomez commented Aug 19, 2023

Attempts to move the generic instantiation to the module that uses it. This should decrease re-compilation times as the source module where the generic lives doesnt need to be recompiled

""".}

type Foo {.importcpp.} = object
echo $Foo() #Notice the generic is instantiate in the this module if not, it wouldnt find Foo
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
echo $Foo() #Notice the generic is instantiate in the this module if not, it wouldnt find Foo
# Check that the generic implementation of `$` for `Foo` is contained in the
# C++ file emitted by the Nim compiler for this module.
# If it isn't the C compiler will not be able to find the `Foo` type.
echo $Foo()

@arnetheduck
Copy link
Contributor

What happens if two modules use the same instantiation? ie HashSet[int]

Also, one thing that significantly increase recompiles is reordering - ie even the smallest change can lead to large recompiles if the call order changes causing the AST traversal to happen in a different order - to fix this, the cgen would have to cache the output of each function and order them deterministically.

@jmgomez
Copy link
Collaborator Author

jmgomez commented Aug 29, 2023

What happens if two modules use the same instantiation? ie HashSet[int]

This is specifically for IC so it would be somewhat cached. The reasoning behind it is to not invalidate IC cache in the first place. Which will produce a new C/Cpp and a road file for the module that holds the generic and will trigger all deps as well.

When IC is there we can see if it is worth it to do another optimisation on it. :)

@jmgomez
Copy link
Collaborator Author

jmgomez commented Aug 30, 2023

Notice CI is broken due to a package update.
IMO we should not rely on the latest version of the GH version of a package for the CI

@jmgomez jmgomez closed this Aug 30, 2023
@jmgomez jmgomez reopened this Aug 30, 2023
compiler/ast.nim Show resolved Hide resolved
compiler/semmagic.nim Outdated Show resolved Hide resolved
@juancarlospaco
Copy link
Collaborator

Very interesting work!. 👍

@jmgomez
Copy link
Collaborator Author

jmgomez commented Sep 5, 2023

Very interesting work!. 👍

Thanks! There are a few edge cases still to cover but getting there

@@ -19,7 +19,8 @@ import
intsets, transf, vmdef, vm, aliases, cgmeth, lambdalifting,
evaltempl, patterns, parampatterns, sempass2, linter, semmacrosanity,
lowerings, plugins/active, lineinfos, strtabs, int128,
isolation_check, typeallowed, modulegraphs, enumtostr, concepts, astmsgs
isolation_check, typeallowed, modulegraphs, enumtostr, concepts, astmsgs,
extccomp
Copy link
Member

Choose a reason for hiding this comment

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

Remove again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, sorry. Totally forgot about it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm, do we still want to move the localPassC right? If so, is still necessary

compiler/seminst.nim Outdated Show resolved Hide resolved
@jmgomez jmgomez closed this Sep 8, 2023
@jmgomez jmgomez reopened this Sep 8, 2023
@Araq Araq merged commit e6ca13e into nim-lang:devel Sep 9, 2023
31 of 32 checks passed
@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2023

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from e6ca13e

Hint: mm: orc; opt: speed; options: -d:release
169755 lines; 9.235s; 620.234MiB peakmem

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.

None yet

6 participants