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

Dynamic rendering mode #324

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Kijewski
Copy link
Collaborator

No description provided.

@Kijewski Kijewski force-pushed the pr-dynamic-rendering branch 3 times, most recently from ad08c26 to edb222b Compare January 28, 2025 19:35
@Kijewski Kijewski changed the title [WIP] Dynamic rendering mode Dynamic rendering mode Jan 28, 2025
@Kijewski
Copy link
Collaborator Author

Kijewski commented Jan 28, 2025

I started the PR before #255, that's why there is a conflict.

Could you have a first cursory look if you like the idea, @GuillaumeGomez? IMHO that could be a killer-feature. :)

How to use:

  • Run the axum-example: cargo r
  • Don't stop the process!
  • Open http://127.0.0.1:8080/
  • Change the background color to red.
  • Recompile (in a new tab): cargo b
  • Hit F5.

Runtime values most likely won't work, because I don't know how to serialize them.

@GuillaumeGomez
Copy link
Collaborator

What's this feature about btw?

@Kijewski
Copy link
Collaborator Author

Kijewski commented Jan 28, 2025

Live-updating the templates, one of the most requested unimplemented features in askama: askama-rs/askama-old#491. (Instead of manually compiling, something like cargo watch should be used.)

@GuillaumeGomez
Copy link
Collaborator

😮

Wow. Ok, let me get through it.


use super::{DYNAMIC_ENVIRON_KEY, MainRequest, MainResponse, Outcome};

const PROCESSORS: usize = 4;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason why four?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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


No. :) Maybe the processor count would be better?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need multiple processors?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without it the templates are rendered one after another.

On my not-so-recent AMD Ryzen 7 2700X system, with four workers and in release mode, the rendering takes about the same time in dynamic mode as non-dynamic: 68µs vs 65µs in he axum example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, another code comment. :D


let mut entries: Vec<_> = DYNAMIC_TEMPLATES.iter().map(|entry| entry.name()).collect();
entries.sort_unstable();
eprintln!("templates implemented by subprocess: {entries:?}");
Copy link
Collaborator

Choose a reason for hiding this comment

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

println! instead maybe? It's not an error after all.

Copy link
Collaborator Author

@Kijewski Kijewski Jan 28, 2025

Choose a reason for hiding this comment

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

STDIN & STDOUT are reserved for communication with the parent process.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds like a code comment is needed. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, possibly even a small UML'like actor diagram.

for window in entries.windows(2) {
if let &[a, b] = window {
if a == b {
eprintln!("duplicated dynamic template {a:?}");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.


let is_done = {
let mut stdout = stdout.lock().await;
stdout.write_all(line).await.is_err() || stdout.flush().await.is_err()
Copy link
Collaborator

Choose a reason for hiding this comment

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

For it to be done, it needs to return an error? Seems a bit strange. At the very least, a code comment would be appreciated here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the connection to the parent process was interrupted, the child process can exit. Severing the connection is the first step in killing a subprocess.

rinja/Cargo.toml Outdated
Comment on lines 39 to 42
linkme = { version = "0.3.31", optional = true }
notify = { version = "8.0.0", optional = true }
parking_lot = { version = "0.12.3", optional = true, features = ["arc_lock", "send_guard"] }
tokio = { version = "1.43.0", optional = true, features = ["macros", "io-std", "io-util", "process", "rt", "sync", "time"] }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • linkme and notify are absolutely needed.
  • I tried to implement the PR without an async runtime, but well, tokio is just too well implemented to invent the wheel twice.
  • parking_lot could maybe be replace with an std::sync::Barrier of some sort. It's far from trivial, though.

@GuillaumeGomez
Copy link
Collaborator

I think I'll need more technical explanations. From what I can gather, we run a server watching files that then sends serialized JSON data to a client which re-renders the needed template files.

However, what are the clients/servers here exactly, how are they rebuilding templates is still something I cannot figure out. ^^'

@Kijewski Kijewski force-pushed the pr-dynamic-rendering branch from edb222b to fc3ebeb Compare January 29, 2025 02:49
@Kijewski Kijewski mentioned this pull request Jan 29, 2025
@Kijewski Kijewski force-pushed the pr-dynamic-rendering branch from fc3ebeb to cfd56ca Compare February 2, 2025 00:29
return Ok(ControlFlow::Continue(()));
}

let _ = values; // TODO

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
@Kijewski Kijewski marked this pull request as draft February 2, 2025 00:30
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