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

Table of Persistance.Reference at the end of the stream #9972

Merged
merged 17 commits into from
May 18, 2024

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented May 16, 2024

Pull Request Description

Fixes #9361 by delaying storing of Persistance.Reference instances and creating their table at the end of the stream.

Checklist

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

  • All code follows the
    Java,
  • Unit tests have been written where possible.

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label May 16, 2024
@JaroslavTulach JaroslavTulach self-assigned this May 16, 2024
@JaroslavTulach JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label May 17, 2024
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

I'm really really happy that the reference loops problems is finally getting addressed! ❤️ I will finally be able to proceed with #9361 :)

The logic for resolving the reference loops may not be the most complicated algorithm, but it is non-trivial enough that I think some comments on what was meant are needed - so that it is easier to understand the whole IR caching system that is getting more and more complex.

I'd also suggest considering moving the -1 reference reconstruction to write-time, not read-time, to make this part of algorithm more localized - thus easier to see 'in whole' at once and thus easier to understand.

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.

I have similar concerns as @radeusgd re lack of documentation for some magic values/array manipulation

@JaroslavTulach
Copy link
Member Author

Thank you @radeusgd for pushing me towards better implementation. Namely, I am very satisfied with d0b0d16 it is clearly better and more easily understandable than the previous version. I believe this PR is ready for integration.

@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label May 18, 2024
@mergify mergify bot merged commit fe28c23 into develop May 18, 2024
37 checks passed
@mergify mergify bot deleted the wip/jtulach/CyclesInPersistance9361 branch May 18, 2024 17:33
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. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support cycles in the serialized IR structure
4 participants