TimelyDataflow / timely-dataflow

A modular implementation of timely dataflow in Rust
MIT License
3.26k stars 272 forks source link

Make `std::time::Instant` optional #577

Open frankmcsherry opened 3 weeks ago

frankmcsherry commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

Ah, oops. I'll take a look!

frankmcsherry commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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: https://github.com/TimelyDataflow/timely-dataflow/compare/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 commented 2 weeks ago

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).