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

Make std::time::Instant optional #577

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

frankmcsherry
Copy link
Member

Ah, sort of what the title says. Many uses of Instant have become Option<Instant> with the intent that an absent instant reflects execution in an environment without time. It a more complicated world we might swap in a generic to replace Instant, but threading that through everywhere seemed complicated, and making the instant optional and various behavior locked behind having a Some instant seemed like a good first dip of toes in the water.

cc: @oli-w

@oli-w
Copy link

oli-w commented Aug 28, 2024

I had a bit of time to try this out today. It looks like the dataflow while probe.less_than(input.time()) never terminates with the worker created as timely::worker::Worker::new(WorkerConfig::default(), allocator, None). It does terminate if using Some(Instant::now()). I'm able to reproduce this without any WASM code yet - try running examples/threadless.rs.

I have got a WASM test harness working though where I can reproduce the "time not implemented on this platform" 😄, will share as soon as I get a bit more time.

@frankmcsherry
Copy link
Member Author

Ah, oops. I'll take a look!

@frankmcsherry
Copy link
Member Author

So, 100% repro. For some reason I thought cargo test ran these, but obviously not in retrospect. If you go back one commit it seems to work, though, and you still get the optional time stuff, while I try and figure out what I've messed up. :D

@frankmcsherry
Copy link
Member Author

Ah yes ok the tiniest of bugs. But very, very consequential. Only claim there is no work to do if both sources of work are empty, rather than if either source of work is empty. Sorry about that. T.T

@frankmcsherry
Copy link
Member Author

frankmcsherry commented Aug 29, 2024

Hey, good news!
Screenshot 2024-08-29 at 3 08 23 PM

This is the result of pasting threadless.rs into a new project, changing one line (ed: and several lines above this, to include things and decorate with #[wasm_bindgen]) to be

            .inspect(|x| console::log_1(&format!("timely says: {:?}", x).into()))

and .. to be honest copilot wrote most of it (except I had to learn that web-sys needs a "console" feature flag).

But, it seems to run vanilla timely programs, at least!

@oli-w
Copy link

oli-w commented Aug 30, 2024

Ahah awesome!! Confirmed that works great for me. I also managed to test out some more comprehensive code I have running DD compiled to WASM, in the browser 😃.

I set up a basic test here: master...oli-w:timely-dataflow:wasm-test (if it helps to simplify some of the boilerplate). I can make it panic with time not implemented on this platform if I try and access Instant::now(), so that should help test there's no runtime usages hidden somewhere. Unfortunately the debugging story is a bit tricky (no debugger), but usually there's a way to isolate the code and run it without WASM.

(except I had to learn that web-sys needs a "console" feature flag)

Hmm I wonder why 🤔, this prelude lets me call log without enabling any flags.

#[wasm_bindgen]
extern "C" {
    #[wasm_bindgen(js_namespace = console)]
    fn log(s: &str);
}

@frankmcsherry
Copy link
Member Author

Your code may work fine! I "had" to do things only because I don't know how anything works, and copilot had me copy/paste some stuff that did not work (e.g. web-sys::console::log_1 rather than an extern fn for wasm_bindgen).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants