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

Data dependent method calling #478

Merged
merged 11 commits into from
Nov 14, 2023
Merged

Conversation

lekcyjna123
Copy link
Contributor

We have currently concept of single caller methods (#416, #425). Such methods allow us to add condition in ready signal which depends from input argument. Such conditions are only supported in single caller methods, because when there are two or more callers there is a risk that we end up with combinational cycle. Goal of this PR is to extend data dependent readiness of methods to multi caller methods.

Current implementation of transactron assume that readiness signal is constant for the whole cycle. So we use it to calculate runnable signal for each transaction. When ready signal depend from data we can decided after scheduling a transaction to execution that method isn't ready. This cause change in runnable and an another transaction can be chosen to execution, which data will be potentially accepted by method, so the ready will be high one more time and the whole story will repeat.

Solution which I propose in this PR is to calculate data dependent readiness before scheduling the transaction, so that we already know that method will accept data from this transaction. The old ready signal is used as global readiness for the whole method for backward compatibility.

Additionally I modified types in method_uses to be more strict. Instead of ValueLike only Record is accepted. This allow to properly type input argument for the user_ready_function.

@lekcyjna123 lekcyjna123 added enhancement New feature or request transactions Transaction framework labels Oct 19, 2023
@tilk
Copy link
Member

tilk commented Oct 19, 2023

Solution which I propose in this PR is to calculate data dependent readiness before scheduling the transaction, so that we already know that method will accept data from this transaction.

I find this explanation very unintuitive. "Before" and "after" doesn't matter for combinatorial logic; only thing that exists are data dependencies. What you are doing, in actuality, is generating separate circuits for the readiness condition for each calling transaction. This relaxes the previous restriction on ready signals, but at the cost of a (potentially small) increase in circuit size.

The idea seems sound; I'll review the PR later.

Copy link
Member

@tilk tilk left a comment

Choose a reason for hiding this comment

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

This change is pretty cool actually, but some cleanups are required.

transactron/core.py Outdated Show resolved Hide resolved
transactron/core.py Outdated Show resolved Hide resolved
transactron/core.py Outdated Show resolved Hide resolved
transactron/core.py Outdated Show resolved Hide resolved
transactron/core.py Show resolved Hide resolved
@tilk
Copy link
Member

tilk commented Oct 20, 2023

The old ready signal is used as global readiness for the whole method for backward compatibility.

This is not just backward compatibility. Both things have a different performance characteristic: there is only one combinatorial circuit generating the old ready signal, but the ready function is instantiated for each transaction calling the method.

transactron/core.py Outdated Show resolved Hide resolved
transactron/core.py Outdated Show resolved Hide resolved
transactron/core.py Outdated Show resolved Hide resolved
Copy link
Member

@tilk tilk left a comment

Choose a reason for hiding this comment

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

Left some minor comments.

@tilk tilk added the review needed This PR needs reviews very much. label Nov 6, 2023
@tilk tilk requested a review from Arusekk November 7, 2023 12:10
@tilk tilk merged commit 261e6f6 into master Nov 14, 2023
6 checks passed
@tilk tilk deleted the lekcyjna/transactron-multi-method-request branch November 14, 2023 12:56
@tilk
Copy link
Member

tilk commented Nov 14, 2023

Merged without a second review.

github-actions bot pushed a commit that referenced this pull request Nov 14, 2023
tilk pushed a commit to kuznia-rdzeni/transactron that referenced this pull request Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request review needed This PR needs reviews very much. transactions Transaction framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants