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

[macro] Add modules created with defineType/defineModule as dependency of current module #11720

Merged
merged 6 commits into from
Jul 19, 2024

Conversation

kLabz
Copy link
Contributor

@kLabz kLabz commented Jul 15, 2024

This makes sure those modules will be pulled from cache when restoring current module.
I can't think of a problem that doing this could cause right now; hopefully I'm not missing something obvious 🤔

This was not a problem pre-hxb (in most cases at least?) because we were keeping modules in the cache even if not referenced from anywhere.

kLabz added 2 commits July 15, 2024 15:25
... of current module.
This makes sure those modules will be pulled from cache when restoring current
module.
@Simn
Copy link
Member

Simn commented Jul 15, 2024

Hmm... intuitively, I'd say that the dependency relationship in these cases should not be symmetric. This would mean that if one generated module changes, all generated modules change by transitive property, which doesn't seem right.

I do acknowledge that we have to restore these modules though. Perhaps such modules should instead become a cache-bound object of the original module? That way we can restore them, but they don't necessarily invalidate so much.

@kLabz
Copy link
Contributor Author

kLabz commented Jul 15, 2024

Hmm right, I was naively thinking these would not invalidate, but that's not taking their own dependencies into account... Will look into cache bound objects for this.

@kLabz kLabz changed the title [macro] Add modules created with defineType/defineModule as dependency of current module [macro] Add modules created with defineType/defineModule as cache bound objects of current module Jul 15, 2024
@kLabz
Copy link
Contributor Author

kLabz commented Jul 15, 2024

I suppose this is still not right:

  • need to take it into account in check_module too
  • those can be skipped in not full_restore cases I think?

Edit: ... which would pretty much make it a dependency again

Considering the following:

  • Main triggers a macro that uses defineType to create Foo
  • Generated Foo module depends on Bar
  • Bar gets invalidated for some reason

Don't we need to invalidate Main in order to get Foo to be generated again?

@kLabz kLabz changed the title [macro] Add modules created with defineType/defineModule as cache bound objects of current module [macro] Add modules created with defineType/defineModule as dependency of current module Jul 15, 2024
@Simn
Copy link
Member

Simn commented Jul 15, 2024

I suppose the problem is that in such cases we can't tell what exactly we'd have to execute in order to define the module again... so yes the conservative approach is to do it via dependencies. I'm quite worried about the effect of this in big projects though, but then again I don't know if this was ever less of a problem before hxb.

@kLabz
Copy link
Contributor Author

kLabz commented Jul 15, 2024

Yeah, will check against shiro projects tomorrow with timed recordings

@kLabz kLabz self-assigned this Jul 18, 2024
@kLabz
Copy link
Contributor Author

kLabz commented Jul 19, 2024

Results so far are very good, with recompilation and completion being 20-30% faster on northgard for example.

... which is a bit surprising

Edit: same result on other shiro projects

@kLabz kLabz merged commit 87b7dbe into development Jul 19, 2024
100 checks passed
@Simn
Copy link
Member

Simn commented Jul 19, 2024

... which is a bit surprising

Yes, I don't think the change here could cause such an improvement...

@kLabz
Copy link
Contributor Author

kLabz commented Jul 19, 2024

Yeah, turns out most of that diff comes from nightlies vs local build (local build being faster)

With both c4cc470 and 87b7dbe as local builds, results are very close (but still in favor of latest somehow, but it's not significant anymore)

@kLabz kLabz deleted the fix/macro_define_add_as_dependency branch January 21, 2025 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants