Define loggers in terms of container builders #615

antiguru · 2025-01-08T13:58:05Z

The logging infrastructure had some old assumptions built in, such as the
container type and the size of buffers. With this change, we defer to the
container builder pattern to re-use the existing infrastructure. This also
allows us to switch the container type to something else if we'd like to do
so.

Signed-off-by: Moritz Hoffmann antiguru@gmail.com

The logging infrastructure had some old assumptions built in, such as the container type and the size of buffers. With this change, we defer to the container builder pattern to re-use the existing infrastructure. This also allows us to switch the container type to something else if we'd like to do so. Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>

Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>

TimelyDataflow/timely-dataflow#615 Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>

frankmcsherry · 2025-01-08T18:29:09Z

communication/src/allocator/zero_copy/tcp.rs

 where
    S: Stream,
 {
    // Log the receive thread's start.
-    logger.as_mut().map(|l| l.log(StateEvent { send: false, process, remote, start: true }));
+    if let Some(logger) = logger.as_ref() {
+        logger.log(CommunicationEvent::from(StateEvent { send: false, process, remote, start: true }));


I've seen a bunch of these, and don't know yet but wonder if log should take a T: From<Foo> where Foo is the type it otherwise expects as its argument.

That's hard because we need a T that can be copied into the container (builder), and that supports From for some argument type. I don't think rust could figure out the in-between type because there might be more than one option. This means we have to specify it somewhere manually....

Gotcha. One option would be to have a Logger<T, CB: PushInto<T>>, which carries the opinions about what you should be pushing in to it. I can't really tell whether it is worse giving up the generality of PushInto, as that would do, or giving up the Into constraint and asking folks to build the logged types more explicitly.

frankmcsherry · 2025-01-08T19:18:05Z

logging/src/lib.rs

-    pub fn log<S: Into<T>>(&self, event: S) {
+    pub fn log<S>(&self, event: S) where CB: PushInto<(Duration, S)> {


Can you say why the Into<T> went away? It feels like a bit of the noise in other files is making up for this. Is it that we shouldn't want to invoke .into() before pushing, or .. some other reason? Or no particular reason?

frankmcsherry · 2025-01-08T19:22:42Z

logging/src/lib.rs

-        if !self.buffer.is_empty() {
-            self.flush();
-        }
+        self.flush();


The code doesn't really match the comment any more. I think it does avoid sending empty buffers because of flush()'s implementation, but also the implementation has a TODO to change that.

frankmcsherry · 2025-01-08T19:26:14Z

timely/src/dataflow/scopes/child.rs

@@ -70,6 +70,7 @@ where
    fn peek_identifier(&self) -> usize {
        self.parent.peek_identifier()
    }
+    #[inline(always)]


frankmcsherry · 2025-01-08T19:34:39Z

timely/src/logging.rs

-    pub messages: Box<dyn ProgressEventTimestampVec>,
+    pub messages: std::rc::Rc<dyn ProgressEventTimestampVec>,


You mentioned that this change was due to needing Clone to work out. I think we should probably at least write a note about this explaining to ourselves what is going on here. Another option (h/t @ParkMyCar) is to have the traits provide a box_clone(&self) method that produces a Box<dyn Trait>, though .. we would need to write the Clone implementation manually. I'm not 100% sure I grok the fall out with Rc, if for example we ever needed to implement Send.

frankmcsherry · 2025-01-08T19:35:25Z

timely/src/progress/reachability.rs

@@ -74,7 +74,6 @@

 use std::collections::{BinaryHeap, HashMap, VecDeque};
 use std::cmp::Reverse;
-


Hrm. Not sure about the formatting, but generally try to space out std vs crate stuffs!

frankmcsherry

Although I think we can merge this, it seems like there is some noise around how we log things, both in the switch from .map(|l| ...) to if let, but also in removing the Into constraint requiring more awareness about the types that actually get logged. Happy to discuss!

Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>

antiguru · 2025-01-09T10:19:53Z

logging/src/lib.rs

-                if buffer_capacity < Self::buffer_capacity() {
-                    self.buffer.reserve((buffer_capacity+1).next_power_of_two());
-                }


We had logic in place that tried to grow the buffers geometrically, but I'm not sure that was still working as originally intended. With the switch to container builders, that logic certainly doesn't exist here anymore.

antiguru · 2025-01-09T10:23:32Z

I changed the PR to introduce a TypedLogger that supports logging types that are Into for some other type that we know how to log. This allows us to remove most of the changes around how we log events.

Also, I followed the box_clone suggestion, which means that we don't need to switch to Rcs anymore, with the downside of additional Clone implementations.

I restored the flush behavior to send empty buffers when nothing had been logged, and drop to only flush if anything had been logged since the last flush.

frankmcsherry

Looks good! I think I understand most of it, but I haven't fully grokked the dirty stuff yet. Sorry if me being confused resulted in PR churn. The other parts look good, and we should move ahead with this, and I can figure out what we need to be true about logging async, though it almost certainly derives from running it and seeing what is bad, which landing this unblocks more of.

* Incorporate breaking changes from Timely's logging update TimelyDataflow/timely-dataflow#615 Signed-off-by: Moritz Hoffmann <antiguru@gmail.com> * Back out of some changes Signed-off-by: Moritz Hoffmann <antiguru@gmail.com> * Upgrade timely Signed-off-by: Moritz Hoffmann <antiguru@gmail.com> * Fix compile errors Signed-off-by: Moritz Hoffmann <antiguru@gmail.com> --------- Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>

antiguru added 2 commits January 8, 2025 14:57

Make the communication log event builder public

8beddc5

Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>

antiguru added a commit to antiguru/differential-dataflow that referenced this pull request Jan 8, 2025

Incorporate breaking changes from Timely's logging update

ae4bb2e

TimelyDataflow/timely-dataflow#615 Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>

antiguru mentioned this pull request Jan 8, 2025

Incorporate breaking changes from Timely's logging update TimelyDataflow/differential-dataflow#558

Merged

frankmcsherry reviewed Jan 8, 2025

View reviewed changes

frankmcsherry approved these changes Jan 8, 2025

View reviewed changes

antiguru added 2 commits January 9, 2025 10:38

Back out some changes, introduce typed logger

3ca33e9

Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>

Cleanup

71754dc

Signed-off-by: Moritz Hoffmann <antiguru@gmail.com>

antiguru commented Jan 9, 2025

View reviewed changes

frankmcsherry approved these changes Jan 9, 2025

View reviewed changes

frankmcsherry merged commit 452226b into TimelyDataflow:master Jan 9, 2025
7 checks passed

github-actions bot mentioned this pull request Jan 9, 2025

chore: release #613

Merged

antiguru deleted the logger_container_builder branch January 9, 2025 12:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define loggers in terms of container builders #615

Define loggers in terms of container builders #615

antiguru commented Jan 8, 2025

frankmcsherry Jan 8, 2025

antiguru Jan 8, 2025

frankmcsherry Jan 8, 2025

frankmcsherry Jan 8, 2025

frankmcsherry Jan 8, 2025

frankmcsherry Jan 8, 2025

frankmcsherry Jan 8, 2025

frankmcsherry Jan 8, 2025

frankmcsherry left a comment

antiguru Jan 9, 2025

antiguru commented Jan 9, 2025

frankmcsherry left a comment

		pub fn log<S: Into<T>>(&self, event: S) {
		pub fn log<S>(&self, event: S) where CB: PushInto<(Duration, S)> {

		pub messages: Box<dyn ProgressEventTimestampVec>,
		pub messages: std::rc::Rc<dyn ProgressEventTimestampVec>,

		@@ -74,7 +74,6 @@

		use std::collections::{BinaryHeap, HashMap, VecDeque};
		use std::cmp::Reverse;

Define loggers in terms of container builders #615

Define loggers in terms of container builders #615

Conversation

antiguru commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankmcsherry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antiguru commented Jan 9, 2025

frankmcsherry left a comment

Choose a reason for hiding this comment