 Hi folks, welcome back to another stream. This is one that has been on the schedule for a while and I completely forgot that it was on the schedule. And so when I realized a few hours ago, I was like, well, I guess we're gonna spend this to go through the remainder of GitHub notifications. If you remember when we started, we were at what, 140 or something in the first stream and now we're down to the only 40 left. So I'm hoping that in today's stream we'll be able to get through most of the remainder so that I can stop having to play catch-up with all of these so much and we'll see how far we get. I mean, we're gonna do the same thing as last time. We're gonna start from the back, walk our way through the notification towards the newer ones and just deal with them as we go. I've gone through and gotten rid of anything that's just like me subscribing to things because I wanna read them so that these are actually like things to do. I've done that for past streams too so that the numbers shouldn't be too skewed. So let's just start opening these and see what we find. It's funny, because now we're starting to get to, we're starting to get to PRs that people have opened during the first of the open source streams. So some of these are like, I put out a comment in one of the previous streams saying so much and implement this and then these, we're now getting to the PRs of people doing that, which is kind of fun. Let's see, so this is in Funtouchini which is the web browser automation library. There doesn't need to wait to register for the entry added event as defined by the DevTools protocol. So it'd be fine if I can instead call something like get logs on the client but that doesn't need to exist either. Currently only implements the web drivers spec and not the Chrome DevTools protocol for that, I think you need to use Steve Pride's 34. Great, which has some support for it. Great, so that's easy. I'd like to keep this library focused on just one protocol and then have some higher layer combine the implementations, combine have DevTools be in a different crate that can be tested and such on its own and then have some higher layer crate like 34 do the combining. Close with comment. That's true. Yeah, if I make a typo and code then file a PR so that I can do it on stream. Great. Oh, so this is the discussion of differential flame graphs that we talked about last time about how the output is a little bit confusing. I guess it's hard to tell exactly what happened. The current implementation matches the output of the real flame graph. And from what I can tell is the intended behavior. Many discussion about this and then running in potential makes sense. It works with this, take stack profile one, take strike profile two, generate a graph using two, colorize the graph using the two minus one delta. I see, okay. So one of the complaints in the original issue here is that if you have a call stack, this is the thing that this is the library that generates flame graphs called Inferno. So in this flame graph, parent is showing the sum of the frames in the children. So sometimes like the frame for parent might be wider than first plus second child to imply that some code ran in parent itself, some code ran in first child, some code ran in second child. And the complaint here is that might be easier to see in the text. If you have a setup like this, the amount of time spent in first child here is 10. The amount of time spent in the second child here is 10. The amount of time spent in the parent is zero, but the amount spent in parent and its children is 20. And in this change here, afterwards first child takes 30, second child takes 30. So that's why they're both marked as red here. But in some sense parent has gotten slower too, right? Because it now takes 60 total whereas previously it took 20 total. And the question becomes, should we highlight parent as being slower or not? Currently we don't, right? So we say that parent has not gotten any slower. The things that have gotten slower are the children. And one of the proposals here was to change that behavior to highlight parent as having gotten slower as well. But it seems like that's intentional. The intent from the original author is that it shows, or the color shows the amount slower, that function has gotten independent of its children so that it doesn't get penalized for its children or penalized for penalized, penalized for its children getting slower. Oops, that's not at all what I wanted to do. I have no idea what I just did. Okay, that goes away. This goes away and then we're back here. I pressed some key combination that I don't know what did. Yeah, Tyler made a magic, it goes brr, you're not wrong. It's also partially because I use the same keyboard now for my work macOS setup and my home Linux setup. And the key bindings are different. So I press keys that do different things and it's frustrating. I'm torn here because the current output is also pretty hard to intuit the meaning of. Maybe the trick here is going to be A to rep, which is going to be mainly around how we present the information in the hover pop-up. Hover pop-up. So when you hover over these frames, you get a little thing that shows here. You see parent, 60 samples. And currently that information is pretty terse. It's hard to really understand what it means on frames that have children. I wonder if we could include the slowdown. The slowdown, I feel like with some labels for which thing got how much slower might actually help a lot. I take, so Brendan is the person who developed the original flame graph that we then ported to Rust. This choice is probably the one that gives more useful data even if it is not the most intuitive one. This is something that this person also, I think was submitting a PR for, which we might get to later. So this is just, nothing has landed yet to change this behavior. All right. Okay, this is for, this is for the IMAP crate. And so the basic premise here, let me pull up the IMAP crate. The basic premise for the IMAP crate is here, let me pull up the actual newest version, version alpha. When you first wanna connect with a client, what you do is you, where's the connect function? Am I blind? Client builder, there we go. So for a client builder, you give the host on the port you wanna connect to and then you do dot native TLS if what you want is a client that uses native TLS as an open SSL, for example, on Linux, or you can call dot rustles and you get back a client that uses rustles to establish a TLS connection. And the way this actually works in practice is that the client type that you get back is generic over the type of stream that underpins it. Like it's generic over the C, which also lets you pass in your own C and everything. And that's nice, but it also means that there's a bunch of generics that get passed around. And it's like unclear how much this matters, especially for IMAP, you don't really care about the high performance here and you probably don't really care about exactly which type is being used under the hood. And once we expose generics here, they tend to bubble up everywhere. Like everyone needs to pass these around everywhere or they need to use like a type alias in their own crate in order to force that this value is always the same thing. So the proposal that came in, and this is from October, 2022, was a client builder and indeed a client that just gets rid of the C parameter. So it still lets you configure which backend to use, but it uses dynamic dispatch instead of static dispatch. So instead of generics, to make the interface nicer while still preserving the same capabilities. And so we've had a lot of conversations back and forth. And this is like an interesting one to follow, I think. It's a change that's been going on for a while about exactly what should we land and how. And let me show you what it ends up looking like. So it looks like this. You have a client builder. You can call dot TLS on it to say, I want this to connect with TLS. And then you can call dot connect on that to say now actually connect to the host. This is just a test. This one though I can pull up. And so some of the discussions we've had is like how exactly do you mask this kind of choice that happens in the builder? Because the builder still has to be able to say like, should I use Russell's? Should I use open SSL? Should I use start TLS? Or just assume TLS directly? Or not use TLS at all? Like assume a plain connection. And so we want to be able to represent these kinds of configurations of the connection without necessarily having to be generic over these types. And so you'll see what we landed on is we have these two enums that describe the connection mode and the kind of TLS that you want. And then down here on client builder, what the builder holds is a field that specifies how you should connect, what your kind of TLS should be and configurations for TLS that we sort of want to expose independent of which backend we use. So we want to be able to have you turn on and off TLS certificate verification without having to like dig deep into like exactly which TLS library we're using under the hood. It's a little misleading that this variable is called no TLS verify and is set to false. Like I would kind of rather it be set to TLS that be called TLS verify and default to true. Let's see. Okay, so we've chosen to deprecate these old builder methods for setting start TLS. And we say instead prefer to just set the mode directly. Seems fine. Right, so one of the other discussions here is you'll notice up here there's something called connection mode auto. And with connection mode auto, the idea is that we will use which port you set and the capabilities of the server to decide whether to use TLS. I want to see if I could find. Yeah, so if the port is 993, then use TLS. Otherwise connect and then detect start TLS. Seems fine. Then we have this danger method for disabling TLS verify. TLS verify. I don't love the double negation here. Oh, these key bindings are killing me today. The double negation here strikes me as unfortunate. Can we name the field and the method just danger TLS verify and TLS verify and have the latter default to true instead. Start review. And then I notice a typo up here too while we were at it. Server says, there we go. Interesting. Is it tough to reject the PR thinking all the work that the issuer have invested? It is. I mean, this PR, they started with basically, and this is often a good way to approach this. They started with a relatively small PR that was just like, hey, how about we do it like this? And then we've sort of iterated on it a bunch. And you kind of want people to come to you with ideas early on, because that way it doesn't feel as bad to shut them down. If like this PR, we've been going back and forth for like almost a year and a half now. And if I now then said, actually, let's not do this, that would be, then I would feel really bad. But I also don't think that's the case here. Like I genuinely think this is a good idea. And if I hadn't, I would have told them earlier. One of the things I did say in the beginning, I think for this one was like, this, it might be that this isn't the path to take, but let's see how it works out. And then we iterated some more and then we were like, okay, this is actually the path we should take. Great. To force use of start TLS. That's fine. Talked about that negation. And then this changes the connection mode to be TLS. If it was set to TCP and then it says the TLS kind, but why doesn't it do that for the other ones? So, oh, so long discussion here. Like, oh, I see. So the native TLS is different from TLS. So TLS is saying require that the connection is encrypted. Start TLS is saying require that we use start TLS. Native TLS is saying when you do TLS and require that you do TLS, then use this TLS library. And then Russell's is the same. Oh yeah, that's a good suggestion. Yeah, I don't love this. Yeah, so they're basically proposing the same thing that I was about to say, which is all of these helper methods of like dot native TLS dot Russell's, they're maybe convenient, but they're also a little weird. Like it's weird that the TLS function sets the mode, but the native TLS function sets the kind. Like it gets really weird. Okay, let's resolve that to get it out of the way. I think I'm sold. I think we should just have mode and kind directly. The, this also end up confusing because it's hard to tell that TLS sets the mode, but native TLS sets the kind. Having just mode and kind is clearer, I think. Shouldn't native TLS imply that it should use TLS? Yeah, so that is one of the things that currently does. And I think that also means that up here in the thing that says TLS kind, if we're now getting rid of the helpers, I think we should have this also specifically set mode to be TLS, if it is set to, if it is currently set to TCP. Currently set to TCP. Even though it may be confusing if someone does, yeah, because I don't want to run into the case where someone sets somewhere else in their code, somewhere that's hard to find, sets mode TCP. And then they call TLS kind setting rustles or open SSL. And then it turns out it's not using TLS because of that setting of mode. And I think it should use TLS rather than start TLS. I think that's right. Yeah, and that's gonna get rid of a bunch of this. It's gonna make it nicer, I agree. And then there's a connect. And connect is gonna call connect with, that's easy enough, domain and the TCP socket. And connection, right? So connect with, I believe takes as down here as a closure that's given the TCP stream and it needs to return something that implements read and write and this set read timeout which we use for a bunch of other things. And then we turn that into a dynamic dispatch object. So you'll see that down here, match on the mode. Yeah, so here, if the mode is auto then if the port is 993, we do handshake here, handshake is just the handler that we get in. So we do a handshake against TCP. Otherwise we do a connection over, we just do a plain text connection to the server. Read the greeting, look at the service capabilities. If start TLS is there, then we do a handshake on the TCP connection. Otherwise we just return the TCP connection directly. And you'll notice the handshake really just means apply encryption, which is a little weird. Like it's weird to me that sometimes it doesn't evoke that function. So this gets invoked in order to construct TLS over the connection, which I think means that it's impossible for it to get here. Like I think that should be unreachable because in the case of auto, you might call the handshake. Yeah, this ends up being wrong. Like if you have this set to auto but you don't have any TLS library enabled as a feature then this will do the wrong thing. The handshake is fine, it'll just return okay, that's fine. But if the port is not 993 and we detect start TLS then we're gonna run the command start TLS even though we did not have any TCP binding. So this ends up wrong. I think this is wrong if the user hasn't enabled any of the TLS providing features. It'll call through to handshake but shake then just returns okay TCP okay TCP even though we've told the server to switch to an encrypted channel. You probably need a config here to not send start TLS if we don't have TLS capability. This is a knit but I'd kind of like for handshake to be called something like TLS handshake and then for us to ensure in connect with that we never call it if we don't have TLS capability feature have any TLS capability features enabled which would then make this be a unreachable rather than return okay TCP. What do you think? Yeah, like this I don't actually think is true because it might all it might get called if the server the connection does not end up as TLS like this framing is wrong because handshake will be called if the server has start TLS or if the port is set to 993 even if you don't have TLS features enabled isn't quite true because we may call handshake currently at least if even if no TLS enabling features are on which I mean it's called despite the connection not ending up as TLS and then TCP just means boxing the connection that's fine start TLS means just do TLS that's fine and TLS means do the handshake great build TLS connection so this is the thing that this is like a helper that uses the appropriately configured library to wrap the TCP stream in a thing that does the actual TLS encryption and decryption. So in the case of in the case of rustles that means constructing a client config, safe defaults, root certificates, creating a config and then connecting okay for native TLS, native TLS connector builders these parts are internal to the libraries we're basically hiding this from users which is nice. What happens with any if rustles is enabled then any calls rustles otherwise any calls native okay so we're defaulting to rustles that seems fine and is that documented up here on the enum where's the enum here TLS kind so we're back in those available rustles used if both are enabled great okay that looks reasonable and then this is that connection thing so this connection trait is just read plus write plus send plus set read timeout and the only reason why we have this trait is so that we can create a trait object or a dynamic dispatch object if you will for the collection of those function or of those traits so you can't do boxed in read plus write plus whatever you can only do boxed in and then one trait and then any number of marker traits so we need to have a trait here that we can then do dynamic dispatch over in order to get the union of those traits but we don't actually want anyone to implement this because the trait itself is sort of like we want to control which connection types people use this with and maybe we don't need to but we have a, so this is the reason why we do it we seal the trait so that we can just do a blanket implementation for any type that implements all of them so if you have a type that implements all of them it automatically also implements this trait and then we use that as the dynamic dispatch object so a connection now which is the thing that a client holds instead of being generic over C it now just internally holds a connection and that connection is a dynamic dispatch object into something that's read and write and then this sealed thing is a rust pattern that lets you make it so that it's impossible for someone else to also implement the trait. Great, not TLS and dot connect Change fine. What's the default up here? I want to double check that I'm not, no not source client, client builder, new. Defaults to mode auto and TLS kind any. Okay, so auto, so it'll generally work for people but it might not default to enforcing TLS. There's a question whether in this day and age we should actually like just require straight TLS by default and then require that you use a more dangerous constructor if you're willing to tolerate plain text. I'm tempted to have two constructors for, or no, rather I'm tempted to have mode default to connection mode TLS really is what most sane clients should be using even start TLS has its problems. Users can then always override that behavior if they know they want to connect to a less secure server. Maybe we have a helper method called dot compatible or something that will set auto plus any letter maintainer. Oh, nice. I've used your thing. That's what we do. We discourage plain text connections. Yeah, that's the same thing I want to do here. I want to make it harder to do the insecure thing, not impossible. Yeah, so the start TLS mode for us will also error if start TLS was not able. Auto will not, but here you see if you try to do start TLS then it does a start TLS call to the server regardless of what the capabilities are and then it does a handshake and expects to be able to do a TLS handshake. So we have the same expectation. Oh yeah, it shouldn't be unsafe. So I'm not proposing that this constructor is unsafe or that setting like auto or even plain text should be just an actually like marked with the unsafe keyword in Rust that I don't think it's valuable because it's not, it's not memory unsafe. I also think I want this. I think I prefer the wording the name. Plans are for this. These tests I've read through before, so that's fine. I'll close that. And this is mostly just changing all these sessions to be connection. I wonder why they even need to be connection. I thought we made this not be generic anymore. Oh, it's still, let's see if I can dig up an example of this. So if you look at client, we haven't actually made client not be generic. So you can still use like the new constructor for example to construct a client over anything. It doesn't have to be a TCP stream. So we're not requiring that you use this dynamic dispatch through the connection type. We're just making it so that when you use the builder and you have all these different TLS variants, the TLS types don't leak out. You can always, if you really want to construct these streams from the bottom, which makes me wonder whether we should update client, the definition of client to set that the default value for C, let me find where's the definition of client. Up here somewhere. Yeah, it's further down, isn't it? Yes, a session here. I think maybe we should add a equals connection so that if you don't specify the generic, then you get the type that you're going to be getting back from the builder anyway. We're so, so close. Up on this. More knits, but they're small. I also think we might want to add equals connection to the T generic type parameter on both connection and just so that in the common case where people use the type, use the builder, they don't have to specify connection everywhere. Having it generic over the stream as a huge upside that it should be really, really easy to do, upside that it should be really easy to test the parser but just using something like cursorVecUate. We already do that actually, but you don't need to have generics for that because remember how the IMAP connection trait is blanket implemented for any type that implements read and write. So that means that you can construct a connection like a boxed in IMAP connection from something like a cursorVec. And so you can still use that for testing. We would just say that, you know, the new from inner takes anything that's generic over all of those sub traits and then boxes it up and then sticks that in the connection field. So you don't actually need the generic for that. Great, done. Nice. This is right. So this is, sometimes people are building their own libraries or even binaries on top of the IMAP crate. And they want the ability to just like essentially test their code without having to talk to a real IMAP server, which means that they have to be able to construct the types that the IMAP corporate returns. So if we look at over here in looking for something like fetch. So on the session type, which is you're connected to an IMAP session, you can call fetch and that gives you back this fetches thing, which gives you an iterator over fetch things which have messages that are in the inbox. But if you are trying to test the thing that's built on top of IMAP, what you'd really like to be able to do is construct this thing yourself so that you can construct one of these without having to run an IMAP server and then pass it to your code that expects to get one of these. So basically the ability to not quite mock out IMAP, but at least be able to produce the same types that the IMAP library is going to produce, but in a way where you control the contents. And so that's what this PR is adding. Great, it's fixed. So there's now a separate, you see SourceLib just adds a testing module and then there's a sub module for the different kind of types that we have or categories of types. And then you see the main thing that it presents is at the moment is this parse method that just re-exposes internal methods we have that takes a sequence of bytes like an IMAP string and turns it in to one of the real objects. Our parse methods are no longer pub, they used to be, but they had kind of wonky signatures, things like how they deal with unsolicited messages and stuff. And all of that is not really stuff we want to commit to in the public API. So that's why they were no longer pub. But then these functions have the very straightforward interface and it allows people to give like, if you got this string from the server, then it would be parsed into this. So it's not a convenient builder for one of these return things of really good fetches. It's not a convenient way to construct one of these fetches, but at least it's possible, right? All you need to give is the IMAP protocol input and it will parse into the appropriate thing that you would have gotten if you really had an IMAP server. Yeah, and then this sort of lots of public sub modules, thing is a nice way to namespace this. So now people can do things like testing, colon, colon names, colon, colon parse. Extended names, ACL response. This seems fine to me. Yeah, we might expand this over time and do things like actually provide builders for these, but I don't think we need to do that straight away. Enables test help features to enable the test helpers feature to expose helper methods to build mock response structures for testing your code that uses the IMAP crate. Yeah, that's for a different PR? Great. Looks great now, thanks. Prove, rolling CI failed, that's fine. So that's some cargo update somewhere. Don't actually worry about that. And I don't think I care about the commit history here, so we're gonna squash this. Exposes parse methods. Confirm squash and merge. Beautiful. So that might go out in an alpha or something. Idle concurrency issues. This one we talked about in a previous stream, which is someone wants to fetch the messages from the server and then do what's known as an idle call. So you can send a message to the server called an idle call, and then the server is going to just keep the connection open and send you a message when a new email is in the inbox or in the selected mailbox. But there's a sort of race condition here where if an email comes in between when you call fetched and when you call IMAP, when you call idle, then that one is not included in your fetch because that happened before, but it's also not a new email as far as idle is concerned because it doesn't arrive after the idle call and therefore it doesn't get sent. And this just turns out to be like a fundamental race condition in IMAP, in like the protocol. Apparently some servers just like, if something comes in between, they just know to include it with the idle, but this is just unspect. Interesting, interesting. Yeah, so they should just be sent later. Yeah, it might be that these end up exposed as unsolicited responses, which we also expose in the API. I'm going to go ahead and close this as I don't think there's much the IMAP crate could do here. But if you think of something, please do let us know. You personally use the IMAP crate for anything. Yeah, so I use it for this little daemon I run that gives me email notifications because I don't run an actual email program. I use mud, which like with mud, I just I don't see my mailbox and nothing is running email wise until I open it. But that means I don't get email notifications which is frustrating. So I've written this little tool called buzz, which just runs a little system tray application that gives you a notification when you have email. And then that uses the IMAP crate. Add inferno collapse GHC prof to handle GHC's profiles because this form is not directly rust related. I'm not sure if you'd prefer this to be part of this report or be a separate crate listing. GHC, the CASCO compiler is it own format for profiling traces. And this adds collapsing for it, nice. Yeah, this is interesting actually, like sure it's not rust related, but the goal of inferno is to be a re-implementation of flame graph. So it's not intended to only work for rust code. Okay, so it adds GHC prof, count bytes, count ticks. Well, this isn't right. If these are mutually exclusive, that needs to be represented. Is bytes and are bytes and ticks mutually exclusive. If so, we should use conflicts with equals bytes and vice versa to enforce that through clap. And so the user gets a nice error if they try. I'd like a bit more detail here. Count which bytes. What does this mean concretely? Also, we should also document that this flag is incompatible with dash dash ticks. And we should document what happens if neither bytes nor ticks are passed. Actually, I want that to be separate. So down here, we should document what happens if neither bytes nor ticks are passed. Probably in the doc string for the entire opt type. So that it appears when the user runs dash dash shop. We have the author of the PR in chat. Great. I'll still leave the comments, they're useful. Main, initialize logger. This part seems fine. All right, and then we got a parser. Why is percent time and percent alloc repeated here? And also there is no ticks or bytes column. Why are and alloc wasted twice? Also, shouldn't there be a ticks and a bytes column? That seems odd. Seems fine. Here too, I think I'd like a bit more detail here because while this is accurate, this is the column. We're pulling the data from. It's not particularly helpful to someone who is trying to decide which source is best suited for whatever they want to do. They would have to go look at the format definitions for GHC prof elsewhere and then come back which is a little unfortunate. Earned costs, the first character and last plus one. I don't follow this comment. What exactly is this? There are three fields in the struct, all of them you size. Okay, so collapse is like the main bit to any implementation of a new format for Flamegraph. So this is the thing that takes like the output from the profiler and turns it into the sort of this, there are this many samples of the code being in this frame. So it's like collapses, which is hence the name. It collapses the giant profiling trace and turns it into this thing called this many times or this many samples rather. Consume the header. Oh, that's interesting. Okay, so what they're doing here is in order to detect whether it's the header line, rather than just like string comparing this to the header line, they're splitting the line by white space, taking the first N elements and seeing whether that iterator is equal to the iterator over these. That's not wrong. What's this break? Oh, I see, so they break here. Oh, nice, okay, so they're reading, like they're basically looking for the header in the file and one might hope that it's first, but that's not always the case. There might be some empty lines in the beginning or something. So I guess what they're doing is looping through the lines, seeing if it's the header line. And if it's not, is this uncommon enough, the header not being the first line that we should issue a warning, question mark or at least print something at say debug level. But if they find the header line, then this is why those are repeated. So module is, so this is the character offset of the start line two, zero, one, two, of the string module in the header line. But why does that matter? Like usually these formats are white space, white space oriented, for example. So like this is saying that the module field is the third column or second, if you start counting at zero of the file, but why the character offset? Cause presumably later lines might have, you know, text at the beginning that isn't all the same width. So this strikes me as odd. Some bytes columns are weirdly aligned. Might help to check out the format. You might not be wrong. Oh, weird. They're actually like aligned? Wait, show me a test file. No, this is the expected output. I want the expected input. Like this one. Oh, so there is expected stuff before the header line. What's also interesting here is that this is, this doesn't actually have all those, oh, this is the first one that's actually the, okay, this stuff we want to skip over. And this is the header line. So let me first then get rid of this because clearly that's not true. It's pretty common for there to be lots of things down there. But this is still white space separated. It's just also aligned, which is wild, like who aligns this output? I suppose, so here's an interesting observation though. The fact that it's aligned means that you don't need to worry about spaces in the value of any given column. So imagine that you have a, the path to your file has a space in it like my documents or something, right? Then in a white space separated format, you have a problem because that space is now a field separator. Whereas if we actually don't go by count the number of white space separated fields and instead go by the absolute character count, then if there's a space in here, it doesn't really matter because we start counting at this offset anyway and count until there. Yeah, in fact, like no location info, if you did white space split here, you would get completely wrong counts. Oh, so weird. Okay. Okay, it's not, the code isn't as wild as I thought. This strikes me as weird. I would just use module here, I think. Start line two is mostly just obscuring what this does. So this finds the offset of percent time. What? Ticks and bytes columns are weirdly aligned. So find the end of the column before. That's crazy. I would like to propose here that we do percent alloc.len and same thing here, I think. Ticks.len. Actually, I want to take that back. I think because I do want to make that change, but this comment is now unhelpful. Let's do this in a better way. Delete. Let's do instead these suggestion like this. And then I'd like to do better than unwrap here. If the user has asked to source from a column that doesn't exist in the input file, we should return an error, not panic. Okay, so once we found this header, oh, I still want to see what's the deal with ticks and bytes. This one doesn't have ticks and bytes, it's almost percent. So give me one that has ticks. These are all results, ha, ticks.prof. Ticks and bytes. Cost center is left aligned or cost. Cost is left aligned, center is right aligned. No, wait. This, okay, cost center is one field and it's set to whatever this thing is and it's indented, but not left or right aligned. Module is left aligned, source is left aligned. No is hard to say, left aligned, I think. Entries is right aligned, present time is right aligned, alloc is right aligned, time is left aligned, alloc is left aligned. Ticks and bytes are centered. That's awful. Why? Why? Yeah, I mean, we can zoom out to have it not do that. Okay, that does, I mean, I don't know that I like it any better. I see, okay, fine. Okay, so they're not centered, they're right aligned, but then, okay, so this brings me back to the question of the code. Like, why are they saying that, okay, let me get rid of some of these in between. Why are they saying up here that ticks and bytes columns are weirdly aligned so find the end of the column before? That only makes sense to me if the value of bytes can go left of the start of the word bytes, which I think is what they're getting at. So when it says weirdly aligned, what they mean is right aligned. So you don't wanna start, if I'm guessing correctly, you don't wanna start parsing bytes at this byte offset because bytes is right aligned so there might be digits here that matter. So that's why you wanna find the end of this column rather than the start of that one. So this is just saying that they're right aligned. They're not really weirdly aligned. Yeah, it's very much intended for human output. I think that's right. Yeah, indeed, yeah, this is right, right aligned. So, here's what I'm gonna propose up here then. I would just say that they are right aligned. Weirdly aligned sounds like they're weirder than they really are. Ticks is right aligned, but bytes is not. Really? Yeah, it is, bytes is right aligned. At least in this file, right? The right side of the numbers aligns with the right end of bytes. And ticks is the same, right? Unless, give me the file for, this is ticks. Do we have one for bytes? There isn't an input file for bytes. So it's this one. But here ticks is zero for everything. Do we have one where ticks is not zero for everything? Result, result, result, result, result. Ticks.proff. So there's not a bytes.proff. It's a little weird that the file called ticks.proff has non-zero bytes, but not non-zero ticks. Oh, that one's non-zero. But these are definitely right aligned from what I can tell, at least. Okay. So after we find the header, then we skip one line. Okay, why do we skip one line? Format, come tell me. There's just an empty line. Okay, thanks. Great. That's fine. And now process the data. Okay, so this occurrences type is a convenience thing we do for people who write collapses. This is basically kind of like a fancy hash map where you can stick things in there and it'll keep track of how many occurrences are in the parents and stuff as well. So you read each line. If the line is empty, if we reach the end of input then we break, that's fine. Pass the string line, trim the end. Line is empty, then we break. Is it impossible in this format for there to be empty lines in the data? As in between rows of profiling information. Cause then this would break early. Otherwise we call self.online. Okay, write the results back, reset the state. Why does it need to reset the state? I think this is something we do elsewhere as well. I don't remember exactly why we have to do this but that's fine. Okay, so online. Yeah, so the lines look like that. And this is awful because it means you have to, like the indentation tells you whether this is a child or not. So when we get a line, we find the number of spaces. Yeah, of course, this is awful. So what is the stack here? What does this actually hold? There's a vector of strings. Oh, right, so there's one space for each level deep in the stack. So the length of the stack is the depth of the previous thing you looked at. So if the new depth is less than the current depth, then we pop this, that means we're in a different parent so we pop the stack. How does that run Kate? But don't you need to handle the, normally you need to like close the stack frame but maybe not in this case. Otherwise, yeah, as we're expecting that either you can drop down any number of stack depths or you're at the same depth but what about one higher? Oh, I have an off by one. So this is looking for the first non-space characters. So is the initial one main? Great. So this one is zero offset. So indent characters is gonna be zero. The previous length is gonna be zero. So they're gonna be equal, great. Then we push main onto the stack and then we walk one down. So now indent characters is gonna be one and the stack length is going to be one because we're a child. Okay, interesting. Yeah, so that checks out. Be non-asky names to take care of the character offset, not the byte offset. So now we also get into this depends on how the alignment code and the thing that produces this output works. Like does it, what does it align by? Graph themes? Cause that's not what characters gives you here. Hmm, by call start, skip while. So you skip leading white space. Wait, but this is wrong though because this would fail to capture contents with spaces in them. So I think this has to not have this take while and instead trim at the end. Won't work for fields that have spaces in them. I think you need to collect all of them, collect until the end and then trim right. So the cost is the string range based on which source column they chose. If that can be parses in F64, if it can't and the cost field is wrong, that's fine. And then we read the cost center in the module. Cost includes self plus calls. Okay, so that means this format is inclusive of children. Which is a weird sentence. Where's the percent input file here? Individual and inherited. I don't understand why all these numbers are zero. This doesn't seem right because so up here we grab which field for time. We grab the first time field and the first time field is individual time, which I assume is self cost, so not including children. Otherwise, this is a very weirdly named field. It's for function and modules so there can't be spaces. Oh, it doesn't include source. That's true, I suppose. Even so, it's nice to have the code just also, if you happen to eventually start parsing the source as well, it'd be nice for it to handle that case correctly too, which I think is fine, right? Oh, it's because you don't record the end of the column. To do that, you'd need to also store the end index of a column, read to the end and then trim right. Probably not worth it, but worth flagging in case this is ever used, flagging in a comment in case this, in case someone ever thinks this might be appropriate for fields with spaced values. I don't think this is true for time. At least the first percent time column is listed as individual, which makes me think it doesn't include child calls. Maybe true for ticks bytes though. Don't lose the one decimal place. Oh, I see, this input always includes one decimal place, but do we, I don't understand, do we pull that back out? So up here, we store the current cost as a U size. I see, so this is saying, it's no longer a percentage here really, right? Because if you're multiplying this by 10, then yes, you don't lose the percentage, but it also doesn't add up to 100 anymore, but that's fine. So it'll be a little misleading because someone puts in percent time and what they get out is, what's it called? Primal, I don't know what it's called in English, but the thing that's more than percent, as in it's a thousandth and not a tenth. So it looks like to push file.trim. I probably wouldn't call this file, even there's a separate field called source, which really is file, or module maybe. Identical stacks from other threads can appear. Yeah, so this is something that Perf actually handles in a different way. In Perf, the thread name is part of the stack. And I don't know whether we want to include that here, but maybe the thread name isn't even included. That's fine, if so. I'm confused about this, oh right, so this is for collapse, so that's just if it finds the start line, that's fine. And then we have a bunch of tests with ticks, including selecting ticks and selecting bytes. This seems fine. All of these input files are fine. Don't really wanna, there's no meaningful way in which I can look at those. It would be good though to actually plot one of these, like do an end to end test that produces an SVG, to see that it actually produces something that looks kind of sane. Like one way to do this is, I guess we can look at this Prof, and so that's ticks, okay, tick bytes. See what it produces. Main.main, oh right, because what it does is it takes the module and then it pens the function with a dot that's fine. Whoa, why? Oh right, because in Haskell you can actually have things with ticks in them. Yeah, so you can have a tick at the end, yeah, yeah, yeah. Because it's valid in identifiers, which is really nice. Okay, and so it produces this with a count. So that seems pretty reasonable. One of my concerns here is that it ends up very long, but that might just be something you can't really get away with Haskell here, with these paths being long. So that seems fine to me. Okay. Yeah, I would love to see a test that actually turns this into an SVG as well, just so we can, it's easier to see whether it looks right than it is to try to parse it out of one of these. Could you also add a test, a test that goes all the way to a flamethrower, to an SVG, so we have a test result that's easier to visually inspect as well. But otherwise this looks good. Although I saw someone mention in chat that GHC has a dash pj option that outputs JSON, and I think that's worth looking into. But I think this is pretty close. Actually, I do wanna include my note about down here. Down here. This assumes that GHC's thing that produces aligned output aligns by UTF-8 code point. But that's, or by Unicode code point. But that's not necessarily the case. It could be aligning by glyph or grapheme instead or grapheme cluster, instead in which case this would not give us the right result. Someone mentioned that GHC has a dash pj flag to produce JSON output. Maybe it's worth digging into whether we should just use that instead. I do actually really worry about this alignment point because who knows whether they're gonna keep this. Like if they treat this format as being just for humans, that also means that they might change it at any time. Like they might just change the alignment of the ticks field. Not to mention this would make us resistant to if GHC changes this seemingly human facing format. I don't think any of the other collapse tests went to SVG. I think you're right. This was actually more just because I think it would be useful. That's fine, I'll leave it out. It would be useful to visually inspect the SVG, but if you have looked at the SVG and it looks reasonable, that's fine for me. One thing we should definitely check out is GHC's pj flag for JSON output. If that can make this job easier and more robust. Also, could you run one of these through all the way to an SVG? Just to give it a visual correctness check as well. If you haven't already, that is great, done. Great. This is one of the things we talked about in a previous one where someone posted some Arata for Rust for Rustations around the syntax I use for awaiting sockets and saying it doesn't quite align with what the real Tokyo library for TCP listeners, for example, looks like. But so this, they seem satisfied with that description. So that's great. Oh, I'm so glad. The next thing I want is I want Inferno to directly parse perf.datafiles rather than the output of perf script and that's going to significantly speed it up even more. Error when iterating over histograms with only zeros. So this is in the HDR histogram crate. This one we also talked about last time. Okay, so the problem is if you have a histogram where you have recorded zero values and then you try to iterate over all the recorded values, it tells you that there are no values in the histogram, which is false. You've recorded zeros. And we had some speculation about why that might be. Problem does not seem to be the more function which is meant to indicate more records with zero counts past the max, but pick returning none as we give it a count of zero. Doing plus equals zero as a particular case is if we only do this, we never update the min or the max. The zero is both the min and the max and those both the first and the last item in the iteration. And we have some code that checks whether we've already picked the index with the last non-zero count in the histogram. Total count index is zero. And this does a skip. Let's see. Okay, so the fix is one line. That's nice. Right, so we have this iterator for histogram iterator. So this is the, there's basically this type that knows how to iterate through all the buckets of the histogram. And it's generic over this type P, which is a way to like specialize that iterator to only count certain things or group the things in the iterator so that we can do things like iterate by percentile, iterate by bucket, iterate by fixed steps or iterate by recorded values. And that's all encountered in this encapsulated in this picky iterator trait. But this is for the general iterator that knows how to walk the histograms. So while we're not at the end, okay, that's easy. Have we reached the end? If the current index is equal to, I need the definition of this current index is equal to the number of distinct values, which should be false. The number of distinct values should be one, right? Because zero is a distinct value. So we don't hit that. Have we already picked the index with the last non-zero count on the histogram? Have we already picked the index with the last non-zero count? If the last picked index is greater than or equal to the max value index. Interesting. So this doesn't seem quite right because this condition up here is whether we are, we should go into this if we are in the, oh, in and have picked the index with the last non-zero count. Yeah, I'm wondering whether the last picked index should be an option here because otherwise I worry that you kind of never get out of the case of it being equal to zero. And so this is why I think it's the implementation of more that's wrong, which is what we got to in the beginning here. The proposal I gave here was, I think it's the more implementation which always returns false here. That's wrong because if more returned true, we wouldn't take this path. And we also wouldn't take the else. And then we would go down here to figure out if the picker thinks we should yield this value, which means that we would call pick. So I actually think this isn't the right fix. So why are they saying that's not the fix? Count returning none as we give it a count of zero. Oh, so they're saying that pick does get called. It just gets called with a count of zero. So this is in fact invoked, but total count to index is zero. I see, and that's because total count to index starts as zero, but doesn't get updated until here. And we never get into this one because we're in that condition. Okay, and we wouldn't get into this one if we just went with more being true. So what this has me worried about is what happens, I think this will mean that you, if you try it or recorded over a histogram that has no entries, that you would suddenly see like one value yielded, maybe you won't because pick would have its count be zero. So maybe this actually is right, but it does feel like maybe it should be option because otherwise you'll never go into this case at zero, which isn't, which is also not right. It could be that you should actually walk the first bucket more than once, which this wouldn't do because the first bucket is sort of excluded from having this path taken, but what would be the effect of that? The effect of that would be on, the only way you would see this is if you had very wide buckets and you tried to iterate by a fixed interval that's smaller than the bucket size, because then you need to call, you need to not take this path because you haven't moved on to the next bucket yet. You're still in the same bucket and you want to be calling more, I think. Not the first bucket, the last. Ah, but last bit index. Right, but zero is the only case where min and max would be equal. It just, it feels wrong. I can't quite put my finger on it. No, maybe the case I gave is the one where this doesn't work. Like you have the bucket size at this wide and you ask for an iterator that walks in fixed widths of this size and then you have no recorded values. Then we would actually want it to call more multiple times within the bucket, but this wouldn't do that for the first of the buckets, which I think then means that we might want this to be the last picked index to be an option and only exclude this path if it's at the none. And then we set it to sum down here to make sure we actually essentially ignore that condition from then on. So let's say we want to ensure we trigger the picker logic at least once to keep the emergent of the loop. Make the initial, prefer to fix to make the initial value last picked in an invalid index being used as it can't be negative. And I don't see a performant API viable way to make it something else, but I'm also fairly new to Rust. So no doubt someone can come up with something better. That's kind. Yeah, I've done this too actually. So this sorry for being slow is often not the right framing if you're an open source maintainer because you don't owe other people your time. So the sort of thanks for your patience is a better way to take it. At this point, I don't feel bad. Like in some sense, I shouldn't be writing sorry because I don't feel bad about it. I know that it's for good reason, but it is true. Okay, so if we did want to make this an option what would happen? I think it's fine for the iterator to hold and make last picked index be an option here because it is not a public field of this type. So I think that's what we're gonna do. The thorough go here is instead to make last picked index an option, option user is that way you can explicitly indicate that it hasn't been set yet. It's not a part of the public API. So you don't have to worry about breaking backwards compatibility. What's also nice is that once you change the type of that value in the, what's this type called? Histogram iterator, the struct histogram iterator type definition, the compiler will guide you to all the places where the code has to be updated to reflect this change. I think you're right that more is not to blame. And I agree with you, it should probably be renamed one day. All right, great. So I can make that as done, which also means that I could mark that PR is done, but that's not even on the page yet. I can refresh it, I suppose. Let me go back and get that. Yeah, this one, great. Update license field following SBDX 21 license expression standard. Oh, that's true, slash is ambiguous in SBDX. That's fine, okay. Great, good, approve. I should probably for this one also have done. Oh, that did run, the check's okay. So this one, I'll wait for these to land and then I'll do a release of Fontachini afterwards just to get it updated on crates.io as well. This one, beautiful, great. So now I can land this. This is a change to the we were wondering site. This requires that I do a bunch of manual work to publish the changes so that I'll do that offline because that's more work that back over here. And same with this one, do a release of that. Once I have a chance. Immutable iterator. So this is a change to stream unordered. We also dealt with this on stream a while back, I guess exactly two weeks ago. I feel like the first life is unsafe. Very much out of my depth in this level. So full review of this function is in order. All other functions remain identical minus some internal rearranging. Nice, okay. Let's go and see what this looks like now. InterpinMuteWithToken, okay. So there used to be a PubStructInterpinMute and there still is an InterpinMute with token is a new one. Oh, this is just, they've reordered where these structs are, which makes the difficult annoying to read. So this one, I see. And this used to be this with these fields, but now it holds one of those instead. Okay, that's fine. IntermuteWithToken, and this is the same thing. Like this now just holds one of those, but the definition of the type, as far as the public API is concerned is the same. So that's fine. That's totally fine. No need to call it out in the public box. So let's just suggest that we remove those lines. And then this is the same. So inter with, whoa. Interpinref, where did interpinref go? Where has interpinref gone? Cause we can't remove that type cause it's pub. Fortunately, we cannot remove this type from, well, oh, is it a two slash comment? Well, the first line is a three slash comment and then it's a two slash. So that's fine. But this interpinref, am I just blind? Interpinref, we're going to have to keep this type as well because it's part of the public API. Unless GitHub is just trying to be helpful and not actually showing me the full diff, but I don't think that's true. Inter with token and same with inter actually. Am I blind? Like, are they defining these somewhere else that I'm not looking? Ah, they weren't pub use. That's interesting. So if we look at stream unordered here and we look at, yeah, so only the intermute and interpin mute are actually exposed, but I think they are in the public API. Oh, I guess they're not. Only intermute and interpin mute are. Then why are they even marked pub? Interesting. Okay, well, that means that this I can get rid of. So there's now inter with token, that's fine. And that's the thing that used to be interpinref of item. Grabs the ID, pulls out the ID. Yeah, so we now have an iterator. So this is a straightforward change of the interpin mute one that grabs out the ID and now includes that in what we iterate over. Iterator for interpin mute. That looks great. Iterator for intermute is now mapping out just the stream. So that's fine. Iterator for intermute with token. Where S is pin mute, get mute, that's fine. So it seems reasonable so far. Do we know that these are exact size iterators? Like where the old iterators, exact size iterators? I guess we do have the length, yeah, okay, great. So the size hint that we give out is self-lens, some self-lens, okay, great. So exact size iterator is correct. Iterator for iter with token is supposed to yield a reference to S and a U size. And this is the thing that used to be interpin ref. So this code, interesting. So this old iterator, so I'm just gonna, I'm gonna assume that my old unsafe code is sound rather than try to reevaluate whether the old code is sound. So if the old code was sound and it yielded pin of S, and this is just yielding S. So this is taking the S out of the pin, pin here again. So you can always project S ref, that's fine. Can you get, yes, if you have a pin reference to T, then you can get ref to get the reference. And that does not require unpin. So it's totally safe to go from a pin shared reference to a shared reference because clearly. So I don't know why the original even bothered pinning these again, it shouldn't be necessary. This can just return the stream. And if it's the case that as we do up here, if it's the case that it's allowed to return references into these streams with the lifetime that we are using here, then it should also be just as safe for it to be shared. This reference though I'm worried about. So here I think we have to do the same thing. Let's do the same thing here as we changed with iter mut which is to not assign to not construct a reference inside task that might not be okay. Once we later in the same function instead just repeat a task in the two places it's used. Otherwise that looks fine. Great, pin mute with token. Yeah, so the iter mute method is just gonna use this wrapper around that which just projects out the project away the token. This seems fine. This seems fine. But I think I want the documentation here. Can you add to the documentation here to point out that this also yields the, I guess it's sort of implied by the name. It doesn't really need to be called out. Okay, yeah, I also would love to have some of these types not be pub, like the, although I think now they actually are all pub, it's just previously they weren't. So I think that's okay. Like now all these four are the only ones that are defined and they're all actually pub. And I was gonna say the documentation here could say includes the token for each stream, but that's very much implied by the name of the function. And I would love to change these around but that would require a major release so that iter pin mute gives you a thing that also includes the token because you can always just project it away yourself. Iter with token. So this is gonna be like iter pin mute with token. So let's look at this one. So the original one does self.headall.getmute and it's only here that we dereference it. Why is this using, oh, we can't use getmute. I'm confused why this even works because head all, if we go back to, what is the original code here? Source, it are pin mute. What? Oh, that's stream entry. I want stream unordered. Where's the definition of stream unordered? Here, head all is an atomic pointer task s. Yeah, okay. I see, yeah. So here, because we have a mutable reference to self, even though this is an atomic pointer, we don't have to do an atomic load of that pointer because we know that we're the only ones pointing to it so we can use getmute on the atomic pointer to just read its value. No synchronization is necessary. And then we dereference what we get back to get at the task, right? The challenge they have is if you have a shared reference to self, then you can't do this. You have to actually load this value. And so this is where you run into a race condition where you might read the atomic pointer to the head and now you have a reference to the head and then someone goes and swaps out the head behind the scenes and deallocates the old one. But you still have a reference to it. And then down here, we now dereference that pointer but it might no longer be pointing to anything that's allocated. So the question then becomes, yeah, it's this function that you're gonna wanna use. Yep, this function implements exactly that. Ooh, no. No. My key bindings are confusing. You know what is the helper? Just below, it takes care of exactly reading out the task pointer and length for you based on A. And that was not unsafe. That should in turn let you make this function entirely safe. Out of your comment. What do we have here? This is a test iter that inserts five tokens and then checks that all of the tokens, ooh. I don't think we want this test. So this test is gonna be order dependent. I don't think we want to guarantee anything about the order in which the token yields the streams. Let's collect the tokens into something like a B tree set instead and that way you can just sort of just call sort on the VEC before you compare it and you should be fine. Same for the test below. Oh, why did they change this? I think they said something about that in their comment. They said, there's an async pink of Tokyo that now exists in conflict with Tokyo. That's great. That's fine. I think then I'm happy. They're quite good. Left a note about how you can avoid the unsafe and then we're almost there. Done. We're making our way through. This one is must be failing for unrelated reasons. That's because we've since improved the test suite. This one I can just merge. Just a, there we go. Great. That's just a follow comment. I can go away. All right. What else we got? Keep them coming. We might not get through all of these. Let's do some that are easy, like these knits and bumps. They get them out of the way. This is in Rustengun. What even was that? That was the wordle silver. Oh, this was the, right. Oh, this is for the fly.io distributed systems challenges. And this was just something like, oh, in the earlier implementations we didn't have this helper yet. So we were just calling Serti Jason ourselves. That seems fine. Thanks. Symmetry. I don't even know if this has CI. That's okay. It's not important. Uh, I believe the drive feature is mentioned erroneously in the last sentence of the second paragraph on page 70. Probably doesn't should talk about some feature instead. Found this in the second printing of the book. So it prefixes erotify with print O2. Is there a second printing? It's a good question. I don't know if the second printing has erota, has the erota from print one applied to it? That's a very good question. I'm going to have to follow up with no starch in here what things they may have fixed or not. It's equivalent to if true, only if there are. Oh, yeah. I mean, this is clearly right. Good catch. You're totally right. I honestly don't know whether they've made erota changes for print for the second print run or not. I'll check. However, I believe this error is in is there in the original print as well. So let's keep this as print O1 to indicate that it's present all the way back to that version. Sweet, but that is indeed incorrect. Okay, I'll leave that open. Right, so this is in left, right about the ability to have fallible operations. So the ability to the left right is a this data structure where you you keep, for example, two maps, rights go to one map, reeds go from the other map. And then you if the writer wants to expose the changes they've made, they swap the two maps, wait for the readers to drain from the old map and then start writing to it. But it means the reads can basically progress without synchronization. And what they want to hear is the ability to have operations against that the writer can do that might fail. And the observation around why this was difficult is that imagine that you have a writer that's writing to one map and they perform an operation that and then the way EV map or left right actually works is that when you perform an operation as a writer what you actually do is you just queue up the operation for when one of the maps has no readers. So it's not actually performed straight away. What that means is you wouldn't actually be told that the operation has failed until one of the two sides empties of readers and it starts to apply the operational log. And at that point it's unclear what you do if one of the operations like in the middle of the queue failed that there's no really reasonable way for you to recover. Let's see. I've serviced a lot of multiple users to send and receive updates to the same data structure. In my particular use case these are tournaments using WebSockets, it uses an update message to server process it and on success the same message is forwarded to all the users so they can apply it to their local states. The received message can fail for a number of reasons. The user was out of sync, the server receives an improper malicious message, et cetera. Each multi-user session is managed by Tokyo tasks to produce the DB bound latency. The set maintains an in-memory copy of the tournament. This ensures that the session can always process incoming messages without waiting on the DB. Persist tournaments to the DB via a separate task that's responsible for persisting all tournaments. After every successful update from a user we send a message to the persistent task to indicate that the tournament has been updated and needs to be saved. Ideally the persistence task holds left-right reader handles to every tournament and will read from them only when it receives a message that the tournament has been updated. Instead there is a system of inter-task communication for retrieving clones to the tournament. Using left-right would both simplify the persistence problem and reduce the number of copies of the tournament that needs to be made. Yeah, okay, so what's interesting here is that, what's interesting here is that you could totally use left-right for this. I guess what I'm curious about is why the right operation has to be fallible itself. Like why can't you read and then check whether the operation would succeed? And if it would succeed, then you do the right. That's the way I would actually solve this problem. You don't actually need the rights to be fallible. Thanks for the context. I may be missing something obvious, but couldn't you just do a read of the tournament state? Check whether the operation would succeed and then only do the infallible right operation if that is indeed the case. Then there'd be no need for fallible rights to be a thing. The reason why rights are enqueued rather than applied directly is to allow is so that when you call refresh, which is the sort of swap of the maps, is actually is so that left-right gets to overlap the time. I'm trying to find the right way to express this. So what actually happens in EVM map is that when you call refresh, so when you swap the two maps, you have to wait for all the readers to drain. Well, you have two options. Either you wait for all the readers to go away from the, let me rephrase that. This might be easier to draw actually, but let's see. You can only apply operations the moment that the one of the two maps is empty of readers and you have two ways to do that. The naive one is that when you call refresh, you swap the pointers and now all the rights are visible and then you wait in refresh until all of the readers have drained from the old map and started using the new map and only once that's the case, you return from refresh and now rights can be applied directly to the map that is out of readers. So that's great. The problem with this is that while you're calling refresh, like while refresh is waiting for readers to depart, the current process is blocked or the current thread is blocked. It can't do anything while we're waiting for the readers to drain. By putting a queue in here, you can actually overlap that time. You can get sort of like, let's call it IO parallelism even though that's not really what it is. So what EV map does instead is at the end of refresh, it doesn't guarantee that there are no readers in one of the maps. When refresh returns, all it guarantees is that all rights are visible to readers, but it actually ends with doing the swap, the pointer swap and then it just returns. So at this point they're going to be readers in both maps, but it returns control to the caller. So the thread is no longer blocked. It can keep doing other operations. Those might be rights, whatever they might be. And then when you call refresh, only then do you wait for the readers to drain and then you apply all the rights from the queue. And so that way the time between calls to refresh is also the time that you wait for readers to drain. And so now you get to do the sort of waiting in parallel with the thread executing, but that only works because the rights don't have to happen straight away. They're sort of pushed back until the next refresh and queued and applied only on a refresh rather than applied directly, is that it allows left-right to... It allows left-right to... Paralyze is like the wrong word, is that it allows left-right to return early from refresh. The time spent waiting for readers to depart one side is time when the writing thread also gets to execute. It's a little hard to explain without a diagram, but it allows left-right to spend less time blocking the current thread overall. Okay. Readme typo. Accessed. Great. Approve. Oops. That's easy. Approven run. Common typo. Yeah, so technically one of these files is generated from the other, but that's fine. Fixing them separately is also okay. Bum's Russell's PKI from 0101 to 0102. This doesn't matter, but I guess it's a high-severity alert. The reason this doesn't matter is because Fantagene is a library, so we only have the lock file checked in because it's a better for CI, essentially. I think it's important for CI, but that's fine. I can take this so that Dependabot is happier. And same thing for IMAP. It's a library. It doesn't matter that this is checked in, but we can merge it. Great. So this is done. This is done. That one's still running. Two different PRs switching to QuickCheck 1. The difference to 9 is that this fixes the capacity properly just by discarding large sizes. Okay. So what do we have here? Cargo lock changes. That's fine. QuickCheck moved from 09 to 1.0. Okay. That's the main change. This one I actually pointed them out. So with capacity QuickCheck, was intended to just see that this VC type, it's like a vector. This is in a crate called a tone that's like amortized resize vectors. And this QuickCheck was just to try to allocate vectors of different sizes and see that they all produce something reasonable. And previously, as in QuickCheck 0.9, when you gave it a U size, it wouldn't generally produce very large values. But now it will produce like any number in the arbitrary range of U sizes, which means that we would try to allocate a vector with capacity like 100 bajillion. But by setting the cap argument here to be U8 and the casting it to U size, we now know that the capacity is going to be passed in as only between 0 and 250, well, 255 inclusive, which is a much more reasonable range and still exercises probably everything we care about. This is just the syntax for generating things changed, which is fine. They're generating the same types U32 arbitrary, U32 arbitrary, perfect. Looks great. Now, thanks, approve. Submit review. And this PR is doing the same thing, but, and I think I know what they're doing here. This one is discarding the result if the capacity is too high. I don't actually want to use discard, but there's some downsides to discarding test runs with prop test or with quick check, which is that it means that it has to generate a lot of test cases that then get thrown away. Whereas when we say U8, it doesn't have to throw away any test cases because they all actually get applied. So this one is basically saying, generate a random U size, and if it's greater than 100, which is pretty likely, generate another U size and then just keep doing that. And I don't think we actually want to go down that path. Thanks, but I think I prefer the U8 approach. Discarding quick check runs in this way is usually not ideal because it means that quick check spends loads of time generating test cases that then are just thrown away. Close with Comet. Sweet. This one can now be squashed and merged. Beautiful. So that one is now done, which also means that... Where is that? This one. This one can now be closed because it was already fixed by that other PR. This one is now fine. Merge. We'll squash it. It shouldn't really matter for this. And we'll squash and merge this one as well. Squash and merge. Beautiful. Beautiful. Wait. What? What? Merge attempt failed. I don't think that matters. There's no conflict here. Great. Thanks. Okay. What do we got? What are we at? 22. Let's do... Oh, these. We talked a bunch about these. Okay. So in Funtuccini, which is this browser automation thing, a lot of the tests are currently things that hit Wikipedia and try to do various browser things on Wikipedia, which is really unfortunate, right? You don't really want it to be set up in this way because it means that if Wikipedia changed their HTML, everything breaks, and we have to update our test suite, which is not great. Okay. So we now have... Okay. We have two different PRs that both implement the same thing. Oh, this person just downloaded the Wikipedia page. Nice. What did this person do? Toilet has just been converted with that issue and currently passed on Firefox and Chrome. It's in two commits. The first dimension changes. The second moves the test to local RS. Two tests that are like input on. Wait for navigation tests. Redirect from a file to Wikipedia. This is a bit awkward with local URLs. The URL after a local redirection may be dependent on where the test was performed. Our proposed solution is to redirect the sample page HTML in redirect test and change it to ends with sample page. I think that's fine. The other does is handle cookie tests. Oh, right. Yeah, setting cookies when you access local pages is pretty annoying. I think it's fine to have the cookie test be external because that's not something that the external sites are likely to change all that much. Yeah, that's fine. Great. All right. So let's look at whether they did the same. What's the local web server? That's basically what it does. So I already have a test harness for running local tests in Fantagini and it basically sets up a local web server that serves just static files. Oh, right. They said they have two commits. One that just moves the file. Okay, that's fine. I believe you placed Wikipedia links with local files. Sample page URL. Why is this just called works? So this one still called current URL. We still do find to CSS. We take locator by ID. It doesn't look like we have something that uses link texts. Oh, this one uses link texts. Great. It seems fine. Yeah, so this too, it looks like they've maybe just downloaded the HTML, which just shouldn't be a problem. I think from a licensing point of view, I think Wikipedia is creative commons, as long as it's non-commercial. And that is the case. I think we're okay. Yeah, so these are pretty straightforward changes. And they simplified some of these. That's nice. Fitter. Yep. That goes away. It simplifies the test a little. Local tester for all of these. This page now has just a little bit more stuff in it. That's fine. Why change the indentation though? It doesn't really matter, I suppose. But why? The indentation makes it so much harder to look at. But it doesn't really matter what's here anyway. Great. I think I'm happy with that. Yeah, this one just downloads all of them, which I don't really want to do. So here's what I'm going to do. We're taking this on. I'm going to stick with 228. I'm going to semi-randomly go with partially because it came earlier. Partially because it came in slightly earlier and partially because they've also trimmed down some of the HTML, which just to check that I'm not saying something false, if I open this page. Yeah, that's what I thought. Trim down the HTML to not actually hold the true Wikipedia, which we don't really need for tests. Tests. Close with comment. Great. Interesting. I can go away. All of the tests used the local HTML file that were already in the test directory. So there's a bunch of... Oh, this is the same person. Great. We have you in chat. Hi. Welcome. Nice to have you here, I suppose. You might have been here all along. Yeah, so there are a bunch of local HTML files already. So if I go to Fantaccini, the repo, and go to tests, test.html, like there's already a bunch of files here that we're using for a bunch of the other tests that have already been converted to be local. It's just that there are some tests that I'll do. I'm happy to just approve this. This all looks great. This looks great. Thank you. I'm telling you, thank you in person as well, so to speak, but it's nice to do it on the PR as well. You can frame it and put it on the wall or something. This looks great. Thank you. As for the two questions, I think it's totally fine to switch the redirect test to use ends with. It's not beautiful, but it still ends up testing what we care about. And let's just keep the cookie test remote for now. We may just not be able to get around that approve. And then I'm guessing this who knows why these fail. The windows test is like super flaky. We run failed jobs. We run jobs. Thank you. I wonder why nightly documentation failed. Here's what I'll do. I'll go ahead and merge this as is. And then we can do the wait for navigation test change in a separate mini PR comment. And I just want to keep as a merge. Excellent. Last time with me having to fix Funtacini the test case is just because Wikipedia changed. Beautiful. Thank you. Alright. Wait, did I not? Mark, this is done. Okay, great. Implement wheel support. I suppose remote and minor follow up. Wheel support. Oh, this was just you need to merge. That's fine. Proving run that. Is there anything I can do better in general? No, I really like that. I like that you split the commits too. Splitting the commit into changes in the same file versus move to a different file really helps in reviewing. Best I can think of, and this is very minor, is in your issue description which I've now closed, of course, because I'm a dumbass. Here. So here this is the trick in GitHub. If you link directly to the lines while opening the file view rather than the diff view so you view the file at the given commit and then mark the lines and paste that link then GitHub will actually expand the code in here and click through it to get to it. It's a little nicer. Also don't be worried about like taking a stance like if you think that this is the best way that we can do it, even if you're unsure then just make that make a commit that makes that change as well and just push it in there and then I as the author can then either go, yes, let's just merge that as well or go actually let's change this part of the PR don't feel like you have to only do the safe things and ask for permissions for the others like a PR gets to go through multiple rounds so it doesn't it's okay for you to put things in there they're opinionated for example. Usually you also don't need these justifications so in general at least when I review a PR I will if something strikes me as particularly weird I'll ask and usually then ask you to include a comment for example to like make it clear not just in the PR but to future people to read the code what went wrong or not when went wrong but why this code looks a little funky but smaller changes like this like I don't need a justification for it seems totally fine oh right this comment I notice that there's some redundant test case and I believe could overall yeah please do I would love to have more coverage on this okay wheel support so this one we can probably just merge once this is good I love finding PRs like this where I've already done the work and all I have to do is click the button what's this right so this is the ability for Funtouchini clients to after connecting to a webdriver host ask or look up what capabilities that host advertised when you connected why is new session response not cloned one of the things I proposed was rather than just storing the capabilities inside of the structure that holds the connection so it's easier to look up why don't we store the entire response from the server whenever it whatever we sort of get back in the handshake and new session response I really oh is that a new type they made oh it's from web driver web driver new service response it doesn't implement clone why doesn't this type implement clone this feels like they just forgot or no one asked them to okay fine yeah stick it in arc that's totally fine there's no obvious default new session response the response is exposed with the getter session creation response I agree that looks too much like your creator beautiful I think value there was a sturdy Jason value yeah gets the response get the response obtained when opening the session which is none of no sessions yet been opened but this yeah unfortunately I don't think we can do this because we we can't directly expose types from another crate we can't actually directly return the type from web driver here as that would mean upgrading web driver to a new major version would also require a new major version of Fanto chimi instead we have the this module which is where we have all these types that are really the same as the web driver types this is what the WD module is for in there we have our own replicas of the web driver types that are under our control and that we can return there we can also derive clone so that you won't need the arc anymore some new session response that now stores the whole thing wait is both the fields are pub too of course they are just a note for when you add that type though make sure to mark it as non-exhaustive we can add fields to it in the future if the web driver creates does request changes done close close close close do you think we are reporting numbers from some standard ADWC C2 instance that are more reproducible in some sense more useful in our cloud driven world I think we should do that this is a bunch of benchmarks for ord search which is like a customized way to do binary search over sorted collections and someone recently revamped a bunch of the benchmarks and now want to run them on a host to see how the numbers have changed but the reality of benchmarks is that which host you run it on also matters so you really want to run it on multiple different hosts and compare the numbers I think we should do that though I don't think it's quite enough or that's not the right way to frame it I think we should do that in practice performance is highly variable on all these shared cloud hosts so often you get numbers that aren't all that reliable and reproducible actually for very low level things like this are very low cycle count things like this as a result measurements from real dedicated hosts are often more useful or representative in practice I would love to get which is all to say that we should gather results from as many different types of hosts as we can and report both aggregate and and report a summary of the results across hosts like in practice when you run performance benchmarks on easy to hosts very often like it's not that the performance is bad necessarily it's just that it's highly variable it really depends on what the other instances that are co-hosted with yours are doing at the time so it's not really the case that if you report the numbers on easy to especially for something that's very like CPU intensive like this that the numbers truly translate very well that's disturbing this is more may need to merge again actually to get 90 97 no 107 what's the PR I just merged to get rid of and to get always way off 228 improvements from 228 never decide which laugh I want to use they're all very different done okay explain why more returns always falls on the record picker more is really more with zero counts but here as is the record we never visit empty bins as it would never be more if we yield a record benefit of the current bin cannot be empty taking the time to add this it's almost certainly going to be helpful to some poor soul maybe even me in the future I love the documentation squash and merge done switch cargo bench for criterion right so this is also an order search for us to move it from using the cargo bench which is like nightly only and stuff to using criterion where actually does benchmarks well let's see just to reflect the changes in nine yeah that makes sense I was saying how we do some tests where we look at how large the data is and we try to make data sets that fit in the fastest but also smallest cash of the CPU the next one up the next one up et cetera and then we label the benchmarks based on which cash we're targeting but their point here is good that like in reality the cash size is very a lot between CPUs so it's not clear that names and these sizes actually map correctly so criteria is just going to give us a graph instead we're like size on the X axis and performance on the Y axis is a fair point and I'm sold there's all the conversation saturated calculations so for U8 and max equals that the resulting sequence will be that's a good point so we have these tests let me see if I can pull up the example of this so we have these tests where we try to sort basically search a bunch of sorted lists of numbers and we try to generate some lists that have duplicates and some that do not and we want to search in both and this one is you want to hit rate of about 50% so we generate twice as many numbers or we search only half the numbers in the set this one we try to stick a bunch of the same number into the collection by just taking the number and then dividing it by 256 because that way you're going to collapse a bunch of the numbers but yeah this is a very good point that currently because of this min up here as we increase the number we're capping it so and then we're just doing division by the same number so we're just going to end up with the same number repeated we should change this one and the one down here this one and the one in construction to instead use int mod 16 times 16 start of view of course changes bit is fixed I think we're good to land this tada this kind of comment is actually mostly for myself it's so that I know that I've looked at this and think it's okay proven run do I answer questions here in the stream? sure depends on the question but in general yes I think I'm about to end though we've been going for a while static unique domains I want to deal with separately this type is not interesting that's a feature I've been waiting for that's as a change to a semester lecture thing that I can do later fix all targets bump MSRV 14 we're so close we're coming up with a variable name foo in a test oh great this makes me very happy someone went through fixed all the fixed all the clippy lints okay okay I think this now doesn't need the surrounding no same here this um I love it when people go through and do things like this makes me very happy they're also very easy to review these kinds of changes replace yep replace takes an array as a pattern clippy disallowed names I want to know what this lint is because this I'm skeptical of um clippy lint documentation very bright um dis allowed names check for the usage of disallowed names such as foo these names are usually placeholder names should be avoided bar is not here since it has legitimate uses that's really funny but where are we using foo like now I'm curious a variable named foo in a test that feels like a related issues I feel like this is a bug with the lint like what will be fine to have a variable called foo in a test um if we're taking the time to do this I've been wanting to sort those warnings out for a while um two small bits that I think we could change further but otherwise this looks great yeah I want to see the offending foo as well I'm curious what that foo is I guess I can search for foo like here for example yeah this is this seems totally fine I want someone to file a ticket with clippy saying that this lint should not apply to tests yes definitely the right call to disable that lint feels like a bug in the lint really to be um just allowing these names in test code great uh question answered yeah do you answer questions you're in the stream yes are you going to your rust I haven't decided yet but I think so um how do you feel about people resolving your comments shouldn't it be the reviewer resolving comments when they're happy with the resolution yeah so this is actually really frustrating with github prs because I don't want the people who make the changes to mark the mark my comments as resolved because realistically I'm just going to have to go through and expand all of them and check if I agree anyway um so this is why I've been using um I'm not using it now but there's a website called reviewable I think uh it's enabled on the open ssh repository I know um review yeah reviewable so this is a really neat tool that um I've used a bit which actually does like good reviews so it lets you say like I've reviewed this subset of the commits um every change or every comment has to be resolved by both parties not just by one or the other so you can't resolve something completely and hide it from the other person um so I like that a lot better I get pretty frustrated with the github one um it's usually fine for smaller prs but once you get large it becomes a mess to deal with um okay what do we got left bumpmsrv since clapmsrv to 170 in front of the longer bills on 164 and it's a minimum of 170 um it's terminal and rustics okay okay restoration 170 so we can list it in the cargo tumble that's nice we had rest of rid of is terminal okay because we can use is terminal from studio that's nice sweet 170 really clap that's so new I don't think that's worth it that's such a new version uh it is to drop a dependency but 170 is very new like this is this is bumping so 164 is from uh 164 is from September of 2022 and 170 is from June of this year so this is a bump of nine months it doesn't I don't think that's worth it um now the good news here is that inferno the binary ships with a lock file that pins clapped before 4.4 so that will still build with the msrv it's just the newest version like if you ran cargo update with msrv but if we make these source changes to get rid of to start using the external trait then then inferno itself also becomes uh inferno itself starts to require 170 so it's no longer possible for consumers to pick an older clap and still have it build um yeah I don't think I want to do this um think I want to bump inferno's msrv to 170 yet um it is too new um with the time being and I don't think dropping the is terminal dependency is that valuable in reality doable in reality the fact that clap 4.4 uh no longer supports 164 doesn't actually preclude preclude anyone from building inferno with 164 as they come up with the right lock file which while annoying is possible is at least possible if we land the uh these changes then users of inferno have to use a newer rust or pin older version inferno I haven't formally set an msrv policy for this crate but I haven't formally set an msrv policy for this crate but in general I'd be looking at 12 to 18 months probably um I think 170 includes a security fix if I've seen correctly so that doesn't actually matter for inferno the library what I want to do with inferno is I want to make sure that people can build it with an older rust if that is all they have including this change wouldn't make anyone upgrade I mean it might give them an error saying rust is too old but usually people are pretty good at keeping the rust up to date if they're able I wanted to be the case that if you're stuck on an older version of rust then you can still build the newest version of inferno um and so that's why I don't want to bump the msrv um okay that's just the coverage report I can go away someone followed up on this during the stream which is I think someone who's watching the stream right now uh okay I'll this one I can review later see I still running uh it's just a custom firefox css it's in my dot files repo on github I think it's called it's github.com slash johnhoo slash configs and it's under gooey slash dot mozilla slash firefox slash chrome slash user chrome dot css hey you are on stream nice um I think most of the remaining ones are actually things that I'm just like following is this which is probably going to have some more changes it's this where we're see I still running uh this which is probably going to be some more stuff uh these which I'm all just following this which is probably a little larger so I don't want to deal with that one right now I think we did it I think we basically caught up I'm no longer overwhelmed by notifications github uh great and I think time wise we did pretty well I think this is roughly where I wanted to end anyway okay so that's the uh that's the end for today I think thank you for for coming out I hope this was uh was interesting I'm probably now gonna sort of switch modes a little bit and not do as many of these now that were caught up I might still do one every now and again um but I'll switch more modes more back to doing the sort of across the rust and implementation rust streams um going forward so I'm excited I'll see you all there and uh yeah streams