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

Add support for threading compiler flag to Rust target #1098

Merged
merged 6 commits into from
Apr 27, 2022
Merged

Conversation

jhaye
Copy link
Collaborator

@jhaye jhaye commented Apr 14, 2022

Based on the given threading flag, the parallel-runtime feature is enabled for the Rust runtime in the generated code.

The way it is implemented right now allows one to override the flag by setting the feature manually in the LF source. There is however no option to disable it the same way.

@jhaye jhaye added the rust Related to the Rust target label Apr 14, 2022
@cmnrd cmnrd self-assigned this Apr 19, 2022
Copy link
Collaborator

@cmnrd cmnrd left a comment

Choose a reason for hiding this comment

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

Looks great!

Do you think we could port the Workers Test as part of this PR to Rust? I think for Rust, the test should also check that threading is indeed enabled (which it should be by default). I think we should also add another test that explicitly sets threading: false and checks that the single threaded runtime is used.

@jhaye
Copy link
Collaborator Author

jhaye commented Apr 19, 2022

In that case we would need a similar feature for the runtime that reports back the number of workers.

Is the manual override from the LF file okay? If the parallel-runtime feature is enabled from a file, setting threading to false won't change anything. Otherwise I'll have to change that around.

@cmnrd
Copy link
Collaborator

cmnrd commented Apr 19, 2022

In that case we would need a similar feature for the runtime that reports back the number of workers.

Do you think this would be possible to add with reasonable effort?

What do you mean by "If the parallel-runtime feature is enabled from a file"? If threading is set to false, then parallel execution should be disabled. If the parallel runtime feature is requested manually and threading is set to false, then we should probably stick to the manual specification, but I think it would be a good idea to also issue a warning.

@jhaye
Copy link
Collaborator Author

jhaye commented Apr 19, 2022

Do you think this would be possible to add with reasonable effort?

Yes, probably. I already opened an issue for it.

What do you mean by "If the parallel-runtime feature is enabled from a file"?

It looks like this:

target Rust {
    build-type: Release,
    cargo-features: [ "cli" ],
    rust-include: "../lib/pseudo_random.rs",
    cargo-dependencies: {
        reactor_rt: {
            features: ["parallel-runtime"]
        },
    }
};

Right now this would enable the parallel runtime independent of the threading flag.

@cmnrd
Copy link
Collaborator

cmnrd commented Apr 19, 2022

Alright, I think this is Ok and we cannot really avoid it. However, if threading: false is set in your example, then we should print a warning.

Copy link
Collaborator

@oowekyala oowekyala left a comment

Choose a reason for hiding this comment

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

(nitpicks)

org.lflang/src/org/lflang/generator/rust/RustModel.kt Outdated Show resolved Hide resolved
org.lflang/src/org/lflang/generator/rust/RustModel.kt Outdated Show resolved Hide resolved
org.lflang/src/org/lflang/generator/rust/RustModel.kt Outdated Show resolved Hide resolved
@jhaye
Copy link
Collaborator Author

jhaye commented Apr 25, 2022

print a warning

How can I do that?

@jhaye jhaye requested a review from cmnrd April 26, 2022 15:43
Copy link
Collaborator

@cmnrd cmnrd left a comment

Choose a reason for hiding this comment

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

Great!

@cmnrd cmnrd merged commit 2bea31a into master Apr 27, 2022
@cmnrd cmnrd deleted the rust-threading branch April 27, 2022 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rust Related to the Rust target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The Rust target should support the 'workers' and 'threading' target properties
3 participants