You're allowed to be evil in performance code
Streamed
Bug #1313 prompted a deep dive into the most dangerously clever code on the site. We explored why the code works that way, figured out why the performance improvement in #1308 was putting comments in the wrong place, tried to fix it, realized more problems, and ultimately reverted it. (And then fixed a small bug right at the end for funsies.)
scratch
https://github.com/lobsters/lobsters/issues/1313
https://github.com/lobsters/lobsters/pull/1308/files
zeal offline docs: https://zealdocs.org/
sneak peak of https://recheck.dev
confidence_order = concat(lpad(char(65536 - floor(((confidence - -0.2) * 65535) / 1.2) using binary), 2, '0'), char(id & 0xff using binary))
confidence_order_path in Comment#story_threads (and more)
a [174, 82, 255]
b [174, 82, 255]
c [174, 82, 255][0, 0, 255]
d [174, 82, 255][0, 0, 255] <- wrong place, reply to a
for arh68: concat in short_id?
c_o_p length is 3 bytes * 31 (depth of the deepest comment)
if using short_id, 7 * 31 = 217
eventually c_o_p becomes so wide the query perf falls off a cliff
q: can I avoid reverting #1308?
revert:
it's safe, it works, it's well-tested
don't revert:
it's faster, and might help with deadlock
more clever special-casing
invalid cached data on story
either: pull back id for final byte OR insert random number
random byte would get 1/256 misparenting
chamlis_'s random byte idea:
diff --git app/models/comment.rb app/models/comment.rb
index e2f1b8ce..cc14a1d0 100644
--- app/models/comment.rb
+++ app/models/comment.rb
@@ -24,6 +24,7 @@ class Comment < ApplicationRecord
dependent: :destroy
attr_accessor :current_vote, :previewing, :vote_summary
+ attr_reader :id_placeholder_byte
attribute :depth, :integer
attribute :reply_count, :integer
@@ -165,7 +166,8 @@ def assign_initial_confidence
self.confidence = calculated_confidence
# initial value for confidence_order because the submitter puts 1 upvote
# skips the overhead of calling update_score_and_recalculate!
- self.confidence_order = [174, 82, 255].pack("CCC")
+ @id_placeholder_byte = rand(256) # exposing for test
+ self.confidence_order = [174, 82, @id_placeholder_byte].pack("CCC")
end
def assign_short_id_and_score
@@ -518,6 +520,7 @@ def plaintext_comment
def record_initial_upvote
# not calling vote_thusly to save round-trips of validations
Vote.create! story: story, comment: self, user: user, vote: 1
+ # touch the comments confidence_order to insert the byte for the comment id
unless score == 1 && flags == 0
# A real comment starts with 0 flags and score of 1.
diff --git spec/extras/bitpacking_spec.rb spec/extras/bitpacking_spec.rb
index 64239751..89a5d953 100644
--- spec/extras/bitpacking_spec.rb
+++ spec/extras/bitpacking_spec.rb
@@ -153,12 +153,12 @@ it "uses the low byte of the id in the last byte of confidence_order" do
c = create(:comment, id: 256 + 9, score: 1, flags: 0)
c.update_score_and_recalculate!(0, 0)
c.reload
- expect(c.confidence_order.bytes).to eq([174, 82, 9]) # id is the low byte
+ expect(c.confidence_order.bytes).to eq([174, 82, c.id_placeholder_byte]) # id is the low byte
end
it "increments correctly" do
c = create(:comment, id: 4, score: 1, flags: 0)
- expect(c.confidence_order.bytes).to eq([174, 82, 255]) # placeholder id before vote
+ expect(c.confidence_order.bytes).to eq([174, 82, c.id_placeholder_byte]) # placeholder id before vote
create(:vote, story: c.story, comment: c)
c.update_score_and_recalculate!(1, 0)
c.reload
diff --git spec/models/comment_spec.rb spec/models/comment_spec.rb
index 6e86599e..e1c35e25 100644
--- spec/models/comment_spec.rb
+++ spec/models/comment_spec.rb
@@ -251,4 +251,24 @@ it "doesn't limit slow responses" do
expect(c.breaks_speed_limit?).to be false
end
end
+
+ describe "confidence_order_path" do
+ it "doesn't sort comments under the wrong parents when they haven't been voted on" do
+ story = create(:story)
+ a = create(:comment, story: story, parent_comment: nil)
+ b = create(:comment, story: story, parent_comment: nil)
+ c = create(:comment, story: story, parent_comment: a)
+ sorted = Comment.story_threads(story)
+ # don't care if a or b is first, just care that c is immediately after a
+ # this uses each_cons to get each pair of records and ensures [a, c] appears
+ relationships = sorted.map(&:id).to_a.each_cons(2).to_a
+
+ # kludge, spec is necessarily flaky - 1 in 256 times, generated ids can be the same byte
+ if a.id_placeholder_byte == b.id_placeholder_byte
+ true # not a meaninful result
+ else
+ expect(relationships).to include([a.id, c.id])
+ end
+ end
+ end
end
"better" approach adding one query to update the comment id byte:
diff --git app/models/comment.rb app/models/comment.rb
index e2f1b8ce..7cc36ae1 100644
--- app/models/comment.rb
+++ app/models/comment.rb
@@ -518,6 +518,8 @@ def plaintext_comment
def record_initial_upvote
# not calling vote_thusly to save round-trips of validations
Vote.create! story: story, comment: self, user: user, vote: 1
+ # insert the low byte of the id to confidence_order
+ self.update_column(:confidence_order, [174, 82, self.id].pack("CCC"))
unless score == 1 && flags == 0
# A real comment starts with 0 flags and score of 1.
diff --git spec/models/comment_spec.rb spec/models/comment_spec.rb
index 6e86599e..a7fb7e24 100644
--- spec/models/comment_spec.rb
+++ spec/models/comment_spec.rb
@@ -251,4 +251,18 @@ it "doesn't limit slow responses" do
expect(c.breaks_speed_limit?).to be false
end
end
+
+ describe "confidence_order_path" do
+ it "doesn't sort comments under the wrong parents when they haven't been voted on" do
+ story = create(:story)
+ a = create(:comment, story: story, parent_comment: nil)
+ b = create(:comment, story: story, parent_comment: nil)
+ c = create(:comment, story: story, parent_comment: a)
+ sorted = Comment.story_threads(story)
+ # don't care if a or b is first, just care that c is immediately after a
+ # this uses each_cons to get each pair of records and ensures [a, c] appears
+ relationships = sorted.map(&:id).to_a.each_cons(2).to_a
+ expect(relationships).to include([a.id, c.id])
+ end
+ end
end
so, really, with 1308: is it ok that story's cached values are invalid?
comments_count, hotness (same again for merged stories)
open question: what is the median time between a new comment posted and any comment in that story getting a vote
dzwdz: that feels bad for first comments on stories
^ especially bad because people won't click in to stories with zero comments to vote + invalidate the cache
does comment sorting even benefit from the confidence calculation?
Lobsters doesn't use downvoting, and flags are rare
should we drop confidence entirely to use score (votes - flags)?
confidence_order -> score_order
one byte is score + 10 [FLAGGABLE_MIN_SCORE; cap at 245]
then other two bytes can be id, eliminates birthdays
q: how much does confidence differ from score?
if we sort by score instead, how many comments change order?
https://github.com/lobsters/lobsters/issues/1298#issuecomment-2272179720
title: you're allowed to be evil in performance code
post-stream:
is OBS volume muting linked to scene? oddness at start of stream
what on earth did I do to alacritty to get it loop scrolling the buffer
vim: fix 'shell' hackery to not lose command
Transcripts are generated with whisperx, so they mistranscribe basically every username and technical term. They're OK but not great, advice appreciated.
Recording
02:47jameslittle230 Mic is hot, btw!
jameslittle230 Hi Peter!
Okay, hey folks.
05:09dzwdz ๐ฆ๐ฆ๐ฆ
dr3ig hi there
dr3ig we can't hear yu
dr3ig hi, now we hear you
Oh, hello.
...15The stream muted for a minute.
That was an odd one.
What was the last thing you heard?
I don't know how long that stayed muted for.
dzwdz i joined at :01 and didn't hear a single word
dr3ig i heard you say hi, and then the cat a bit
I think OBS is being a little unreliable because I had thought it was muted and then the mic was hot.
Maybe it's a scene specific thing.
nanerbag Good day
I will have to play with that.
jameslittle230 Last thing I heard was some chatter in the background during the blueprint screen
Joined it on one and didn't hear a single word.
Weird.
jameslittle230 and then silent when you switched scenes, maybe?
It is not nice to not be able to trust the OBS software volume control.
That's no, no bueno.
Hmm.
So anyway, silent when I switched scenes, maybe.
Oh yeah.
So let's put that on the post stream.
06:20I'll just investigate that later.
Anyways, thank you.
It's kind of... You know, I know streams develop their own...
I guess lore is almost the right word.
Their own running jokes.
And unfortunately, it looks like audio issues are going to be one of mine.
I guess that's par for the course of the Linux desktop.
The actual thing...
So the story I told about the cat was that...
He gets fed at 9 a.m. exactly.
And I say exactly because he has an alarm clock for it.
So he was in here hassling me because I walked into the room before feeding him.
jameslittle230 How does he do when the time changes for daylight savings?
And he was a little confused by that, that usually it's me feeding in the morning, but not today.
So it's very funny to have a smart cat who can learn things like alarm clocks and tricks.
does he do when the time changes for daylight savings if we see it coming we will adjust the alarm clock five minutes each day and then you know that just kind of smears the time change and it's fine if it's not well he's a lot happier with the time change where food comes an hour early that one he doesn't get bothered by it's only the time change where food comes an hour late that he gets a little put out of shape
yeah that's the one we try to i never remember it spring forward fall back yeah so it's in the fall here that we try to step it by five minutes each time so there was oh and i don't know if i said it when i was muted but i missed monday because i was offline in the woods with some friends and you can see a picture of the literal stream i was hanging out by
on Mastodon or Blue Sky.
The, I guess it was 10 days ago stream, something like that.
I merged this pull request to pre-compute comment scores on initial insertion.
pushcx https://github.com/lobsters/lob…
And since then, there have been a whole bunch of bug reports, most especially, let's go to, let's grab this.
Yeah, rendering wrong post in response.
So I will share this in the chat and I will stick this in the, let's grab this URL too.
I will stick this in the scratch file.
So it goes into all the notes and all that kind of good stuff.
The report was really clear, which I super appreciated, but basically a bunch of these comments have been appearing in the wrong place.
And this
Bug is almost certainly connected to this pre-compute comment scoring because comment scores are how they get sorted and all of this.
I'm pausing because I can't summarize it.
This stuff about confidence order is related to sorting and it's all a big performance improvement.
And then 1308 is a performance improvement on top of a performance improvement.
So it's kind of not surprising that it's hard to get reliable at first.
And confidence order was in a sequel and trees don't mix very well.
Confidence order was an attempt to get comments out of the database in a
single pass without having to say something like select all of the top level replies select all the replies to any reply we saw like that's that would obviously murder performance and the way the code previously worked was it would grab all of the comments on the story and then on the ruby side it would loop them until it figured out what the nesting was and so it would do one pass through to find all of the thread routes and then it would do more passes to find the
replies to those and so on and so on, and it would build up the tree in memory.
And it was...
I'm sure every developer is thinking, like, what's the big O complexity of that?
The big O complexity of that was something...
I want to say it was pretty smart and it was based on...
10:52It built a hash in memory?
God, I would have to dig back in this code.
I want to say it built a hash in memory of...
messages to an array of their responses i don't remember which direction the arrow pointed or it was a hash of comments to their parents and i want to say it looped the comments twice rather than like a n squared or the depth of it or anything so like performance wise it wasn't bad but it wasn't great because
Looping over comment records is the sort of thing the database should be doing.
So I inserted this big confidence order thing to say, okay, let's have the database understand at least enough of the tree, because we can do that with a recursive comment table expression, and it's reasonably performant.
We'll have the database understand that and give us all the comments in order so we don't have to loop them and build up the tree in memory.
This 1308, Byroot contributed this to say, hey, these initial values, what's happening is you are inserting these dummy values and then you call updateSquareInnerCalculate.
And updateSquareInnerCalculate fires off, I think he said eight queries.
dzwdz i wonder what's the story with the most comments
I don't know that I quite counted to eight myself, but plenty.
dzwdz how much the big o complexity really matters
And so we also have a,
different bug that's still open because I can't quite lock it down.
I don't know how to pronounce your name, dzwdz.
If you want to know the story with the most comments or the story with the most comment depth, figure out what's interesting to you and we can run that query in a minute.
One of the reasons
Biroot was looking at that.
Oh my God, a typo in the subject.
I have to pull up a personal browser off screen and fix that.
That's super irritating.
There we go.
When there are
Lots of comments getting posted at the same time as votes infrequently.
And I say infrequently, maybe two or three times a year.
The app throws an error because MariaDB has deadlocked because the voting code and the comment insertion code both touch the comment and
vote tables but in different orders and when two queries lock tables the same tables in a different order that's called the deadlock and MariaDB goes well one of you queries gets to lose and it's always the vote that loses I don't know if MariaDB is being really clever and realizing well you have a smaller transaction or it's just there are 10 times more votes than comments and so the odds are it's going to be a comment that gets killed
or I'm sorry, a vote that gets killed rather than a comment.
But that's why it hasn't been a super hair on fire emergency because dropping two or three votes a year is not a big deal.
But Beirut mentioned that they had done this in part to reduce the number of queries.
They didn't expect it to solve the deadlock, but there's also the chance it accidentally solves the deadlock because this touches a bunch of those code and vote tables in the same
transaction in the same neighborhood.
And that is a lot of context here.
Yeah.
Let's take a second to answer your query.
15:04dzwdz yeah no i did not prepare it :p
So I'm going to bring up the console and we have the, let's not count that on screen.
...15dzwdz wait no that's not what you meant i'm stupid
dzwdz nvm lol
arh68 can you also query like Mean Average # Comments per story
oh that's all right let's actually read these commands before i hit enter on them that's old stuff from a previous stream so here we go we can just kind of so i'm happy to write this query because i can do it off the top of my head rather than
arh68 idk how to do 1-liner medians in sql lol
Yeah, I can calculate mean average comments per story.
Let's do this one.
Select stories.shortid count star.
Doesn't stories have a comment count?
Hold on.
I believe this has a...
yeah there's a cached column here to avoid exactly this joint so this these are going to be very easy queries so we'll say select short id title comment account from stories order by comments oh i don't have read line correct comments account from stories order by
jameslittle230 Philosophically: do you only treat โmariadb has been deadlocking in prodโ as a bug or would you also treat โmariadb has the potential to deadlock in this scenario that we havenโt hitโ as a bug?
So here are the stories with the most comments on them.
Yeah, this feels about right.
dzwdz honestly less than i was expecting
dzwdz i thought we had one with like 700
We just had, yeah, I saw this thread maybe a week ago, and I was like, oh, this is a strong contender for a ton of comments.
And I guess my intuition was right there.
Yeah.
Unsurprisingly, the top threads are meta threads.
Philosophically, do I treat MariaDB has been deadlocking as proud as a bug or would I treat it as has the potential to dialogue in this scenario?
17:15dzwdz i mean evidently we haven't
I don't think we've had a thread with 700 comments like
I'm actually surprised.
I would have guessed that Microsoft buying GitHub was on here.
That was a huge thread, but I guess it was just huge for merges rather than huge for comments.
Well, so DZ, it is also this comments count.
Oh, well, let's just check this backdoor one.
It's possible it doesn't count merged comments.
And I think it does, but I don't remember off the top of my head.
But we can check real fast because this story I remember had, okay, so that 306, resizing the terminal, cut it off.
That 306 does include merged stories.
So you're not remembering a 700 story where we're like summing all of the short IDs.
Yeah.
Maybe I'll do that on stream in a couple of weeks of trying to clean up the story merge database model.
I've been thinking about it a long time and griping about it because it's an infinite bug factory.
But at least in this case, it's not an issue.
So James, I didn't miss your comment.
I'm not sure what you're asking, though, because MariaDB always has the potential to deadlock where anytime we're doing a transaction with
hitting multiple tables, it is possible that another transaction wants to lock in the opposite order and is going to cause issues.
So I don't really consider that a bug, because it's always possible to deadlock.
If you're asking in the more broad sense, because I guess really where I'm going with that is the only alternative to something like that would be an entirely different database engine, something like MongoDB, where there
I don't want to talk out of my ass.
I don't know MongoDB well enough to say it, but I don't think it has that kind of multi-table transactions.
Maybe a MongoDB expert can answer, but I guess what I'm trying to say without a specific example is that we'd have to have a very different database technology to avoid deadlocks, and I am okay with every database is going to have some amount of corner cases and limitations.
jameslittle230 yeah, sorry, bad example, just wondering when something elevates in your mind from potential limitation to actual bug?
These deadlocks, I think this is like the second one I have seen in a SQL database in prod in, I don't know, 25 years of working professionally as a programmer.
So that's a pretty good failure rate to me.
I am sure there are different uses that could have deadlocks much, much easier.
but it hasn't been my problem.
When does something elevate from a potential limitation to an actual bug?
When I get an exception in prod, really.
Or I get a bug report.
And then there's that, you know, little bit of investigation where...
Does the user understand what's happening?
Are they correctly characterizing it as a bug?
Most of them, especially with a tech-savvy audience like Lobster's has, are actual bugs, but occasionally the copy is unclear or the site norms are unclear and somebody thinks there's a bug.
One place where... One thing that happens fairly regularly, maybe...
five or six times a year is somebody who really wants to do marketing comes on the site and then they post a link to whatever their startup is or whatever their project is and they put like 20 tags on it and you can tell that's them thinking tags work like other sites on lobsters tags are mostly there to filter out stories and so by adding 20 tags not only do they really stand out and look bad
They have hidden their story from the most number of people.
So it's kind of funny to see that.
jameslittle230 makes sense, thanks for indulging me :)
I don't consider that one a bug because it's a user misunderstanding how the tool works.
But I think it's interesting that there is a gray area between is there really a bug.
So ARH, you asked for mean average comments of stories.
So let's...
I think there's...
Can I just say average comments count from stories?
I want to say that's a SQL function.
arh68 HahaCat avg(..) ya i think
And then it automatically, yeah, there isn't a need for a group by here.
Is it lobster?
Is it AVG?
There you go.
I would bet the median is like zero or one though, because we get a whole bunch of stories where, yeah, is there a,
Anybody know offhand if MariaDB, I can bring, we can just grab that.
So here's a Zeal, my doc set manager.
We have my SQL median.
Yeah, it doesn't look like it has a built-in median.
Does it have a mode?
Yeah, not that kind of mode.
I want the mathematical mode.
dzwdz wait what's that program again?
If I look for AVG, are there anything?
Is there anything else on this page about it?
arh68 i would order by, then select N/2th
Yeah, I could give you the standard deviation, but I couldn't give you the median.
arh68 idk how else
And that kind of makes sense because median is a little more complicated to calculate.
ARH, if you want to take a second.
Oh, what's that program?
That program is Zeal.
pushcx https://zealdocs.org/
Let's find it real quick.
I think it's just Zeal Docs.
Yeah, it's an offline, I'll share this here in the chat.
It's an offline documentation browser.
And it's one of those where somebody made a really nice commercial Mac app for offline documentation browsing.
And then the open source folks were like, oh, let's knock that off and do the exact same thing.
And I tease them a little, but it's a very, very open source thing where it's like, oh, there's a successful commercial thing.
dzwdz this looks dope, thanks
Let's make our own version of that.
Dash.
Dash is the thing.
That's the Mac OS app that's a little more polished.
I do this, I think I started using this like the day that Zeal came out.
I saw the link and I was like, yes, because I had a laptop at the time and I did a lot of coding on the laptop.
And especially, this project has been running for
Something like a decade, I want to say, right?
Where's your GitHub?
But I want to say something like a decade.
dr3ig it has docs for ruby, ruby2 and ruby3 :)
And at the time, coffee shops had much less active, much less reliable Wi-Fi.
And so having an offline doc set was super useful for me.
Is there a... Yeah, docs for Ruby 2, 3...
I really think the world of Zeal.
It's a super useful tool.
I'm aware a lot of people use it for things like plain coding.
I suppose I could have coded while I was on my offline stream vacation, but I was very much offline, so I didn't write any code this weekend, which was unusual.
I usually try and work on a personal project at least for a minute.
It doesn't want to load.
That's okay.
So we'll just have to go with my gut recollection that Zeal is something like a decade old, and I've been using it about that long.
arh68 2013-2024 indicates youre right
So let's grab that and put that in the... Make sure that gets highlighted in the stream archive.
Oh, 2013.
Hey, implies I'm right.
arh68 it was at the bottom of the page
Yeah, occasionally my memory works.
I don't have the sharpest memory, but I have that rough feeling it's been about a decade.
So do we have a... Oh, it was at the bottom of the page.
Thank you for catching it.
easeout seems some DBMSes do have a mode aggregate function. without it i suppose you just order by it
Let's see real quick if we can just find someone who's written cross-platform.
That's...
arh68 smellsl like T-SQL ya
fine i've never seen this top 50 is that that must be a my sql thing right or i'm sorry ms sql oh and here's somebody saying here's a better one it also requires sql see it's a frustrating thing that sometimes people
They say SQL, but whenever I see SQL with a specific year, unless that is the year of the SQL standard, I assume they're talking about, yeah, new in SQL Server.
Frustrating that Microsoft named their database that.
Let's... Really?
You just told me there wasn't a median function.
MariaDB.
Did I typo median?
That would be a little embarrassing.
Select median.
No.
Oh, median is a window function.
OK. Well, we don't have any partitions, so we can just say.
Select.
Ian.
Where's my and then I think.
OK, that wasn't useful.
What do I say here to group them all?
I want to say partition by ID.
Now, what I really want to say is group, not partition.
What if I just say partition by one?
Anybody feel real confident in window functions?
I know a little.
Yeah, that's not.
That's not at all what I intended.
So in this example, we are partitioning by title.
And I would just want to partition by true because I want the median for all.
And I haven't done a ton with
window functions and so that's why i'm flailing a bit here because what i want to express is that i want a window all and the default over is like rows preceding and no rows following yeah partition by and if i
don't if I just say over empty.
easeout Is there a more general percentile sample function?
arh68 i guess the median is 1? select distinct
Yeah, it's just giving me it's calculating like a running median, I think.
arh68 if the mean was 4.2 median being 1 is believable
I don't think the median is one.
I think it's, yeah, it's, so I'm getting this implicit unbounded proceeding, unbounding following or unbounded proceeding to, can I just say range all?
Yeah, no, I can't say that.
29:49Yeah, so here's the example of if you say nothing, the sum is run over the entire data set.
Oh, okay.
being the median is believable.
dr3ig try where comments_count > 1
All right, then I guess it's referring, let's say distinct, because if it's just telling me one for everything, that is plausible, but then each row would have the same median value.
I'm just, I'm really suspicious of this 0.0000,
dr3ig or some other where
arh68 mode is most popular
I guess median is the most popular value, and I'm thinking of the other arithmetic means.
All right, where comments count, comments comment.
Oh, yeah, the median is six.
All right, yeah, I guess that's true.
So I had guessed that the median was probably gonna be zero or one, and it looks like it's one.
Do we also have a,
A mode as a window function?
I'll just try it.
No.
Shame.
Median.
Or wait, is mode the one that's also average?
arh68 mode would have to be 1, no ?
No, that's mean.
dr3ig can you count(1) where comments_count <= 1
Taking me back to junior high.
dr3ig to make sure
All right.
arh68 if median is 1, half are 1
So with that bit of insight,
Can I count one where comments count is less than or equal to one?
If the median is one half or one.
31:49So that's 6300 or 63,000. And then, yeah, about the same on the other side. Yeah, that's a good gut check. Good thinking there. I like that. I really appreciate when I'm unsure and unconfident in the tools doing those kinds of checks. So thank you for thinking of that. It's nice to get that confirmation that we're generating roughly what we expected out of that. Or we are indeed seeing plausible answers is maybe the better way to put that. However, speaking of seeing implausible things, we have been seeing comments not rendered to their correct parent. And it is almost certainly this 1308 because it was deployed like three hours before people started reporting the bug and it touches that exact code and no other code touches comment sorting. So I feel highly confident that this is the culprit, but I wanted to spend a minute playing with it and seeing if we can, number one, write a test that reproduces the bug, because then we have a really high confidence. You know, we have certainty that it's what caused the bug, but then also we can look at, is there a way to kind of rescue this work? Because I would like to save all of that selecting on comment insertion. So one thing that showed up in the rendering the wrong post and responses is if you scroll down, Oh yeah, I mentioned it in here like I suspect, it is something about these comments being replies. Because. I was just kind of thinking about it, while looking at the stream know before and. There was. I don't need a terminal Anita of him.
34:12Oh, that is the wrong oh i'm on a branch sneak peek alright so eagle eyed people oh yeah so recheck is the gem i'm working for working on for database integrity and.
dzwdz heh, ddate
i've been.
hammering it out and it's kind of fun it's a tool for for making sure you have correct data in your database it could almost be used on this bug because d-date yep i've had it at the start of every terminal forever since i've had a linux desktop in i think the first stream i talked a bit about discordianism so if you jump back in the stream archive on my blog you'll see some mention of the book principia discordia
could almost be useful in this scenario of our comments appearing under their correct parents but the tool is at like you know 0.0.0.1 rather than something i can ship so let's let's get off of this branch let's oh i'm in the middle of
jangomandalorian ๐๐ผ Hi everyone
konradpetersberg yo
konradpetersberg what up
yeah so i've been using lobsters because it's a big real site with real complex data well you know why not let's go ahead and run it we'll do a sneak peek so you write these checks of your data and i want to say message oh it's going to dump but if i if i run this on stream it dumps the whole object and there's going to be stuff like users emails and other things
Where's a good one?
Keystore passes.
Hey, Conrad, welcome.
konradpetersberg bless me coding god
Here's an idea of what it does, where a check looks at your database table and then each one of these dots is a thousand records in your database and it checks that they're correct.
konradpetersberg emmagr1Clap
If you have ever seen the, bless you, Coding God, may your bugs be few and obvious.
The, if you have ever run a Rails app in production or really any app, you probably know that once you get past a couple hundred thousand records, you have records where like you load them out of the database and they say that they're invalid and it feels impossible, but it's a real thing that happens.
I gave a talk on this a couple of years ago.
It's fundamental to the way ActiveRecord and most other database libraries work.
Even setting aside the fact that you can revise your code validations, but not remember to run a migration to fix all of your data in the database.
Even setting aside that obvious check.
I'm trying to think which of these is not going to include personal info, but one of these has bad data in it.
Oh, this table must not have a column on it.
That's running a lot slower, and I don't want to run it.
All right, let's go ahead and...
load away that's all generated code so i don't need it so that aside sneak peek aside i hope to have a beta of that out soon and that's here i will put it there's a mailing list if you're curious about it so with that aside confidence
37:50yeah so this initial confidence is the code that was touched by 1308 and when i think about what confidence is in this context confidence is this formula down here it's based on a classic famous blog post by evan miller and how Reddit sorts comments. I will not be explaining the math on stream because there's a lot of it. But the basic idea is there's a big difference between a comment with a score of 1 because only the submitter has voted it up, and a comment with a score of 1 where 10 people have voted it up and 10 people have flagged it. Numerically, they'll sum the same, but they mean very different things. And so this is a function for getting at that idea that every vote is information, and rather than sorting by the naive score, we should use all of those votes. That becomes the comment's confidence, which is basically it's ranking each sibling level. So if we have a parent, you know, parent comment A, and then it has the replies B, C, D, confidence is what order are these being sorted in? And so that's part of why when I see the bug that people are reporting is that like D is not, especially when, here, let's just pull up that useful screenshot.
39:46So to adapt this, what we're seeing is we have A, B, B has C and D. Yeah. However, D is actually a response to A, not B. And so when I see this thing of, oh, the comment is sorted under the wrong parent, that's really conspicuous for, oh, the confidence order is wrong.
40:33So this this big comment that's like 100 words is me explaining the clever thing that confidence order path does to allow the database to use a recursive common table expression, this is the kind of.
I'm so conflicted on this code.
On one level, I love it.
It's very clever.
It does bit packing, which is always fun for old assembly coders like me.
On the other hand, it's not 100% reliable.
It is only like, I don't know, four nines reliable.
which is maybe not good enough for core site functionality, like showing comments in the right places.
arh68 "here be dragons"
And when things need these kinds of giant comments to explain what's going on, the hair stands up on the back of my neck saying, oh, that's probably too complicated.
Maybe that's too complicated.
41:46Here be dragons.
Yeah, that's very much what this is.
So confidence order is those three bytes that 1308 touched.
So let's jump up to an initial.
So this says that the confidence order is these three bytes.
And then this comment explains what's going on with these three bytes.
The first two bytes are an unsigned integer so that it's sort of the comment order.
Just basically that.
Boy, how did this cause this bug?
Because it only should show up when the confidence should be the same value
And this is all tie breaking between the depth.
But if there are.
Top level.
If the top level comment has the same.
So where this is going is.
This idea of a confidence order path.
Which is this big.
Fairly wild thing where.
We are doing a bunch of string interpolation.
arh68 should the confidence depend on the parent comment?
So what's happening here, and I'm going to do this over here, is you end up with... Let's just jump down and show the query.
Should the confidence depend on the parent comment?
No, no, it doesn't depend on the parent comment.
So if we say, where's the simplest one of these?
There's recent threads and then there's story threads.
So this is the big workhorse query that grabs all of the comments for a story.
And then there's this idea of a recursive comment table expression that says, okay, we wanna find
All of the comments let's start with those.
At the top level so that's where parent common ideas know that's all of these top level replies like this one by emperor.
And param I know his last name and.
44:38then union that to let's find all of the comments that are replies to those and so on. And doing this in the database is of course faster than yoinking it all back to Ruby and doing it again to, I think it was ARH's comment of if there's only a couple hundred, does it matter? Not hugely. One of the reasons I waffle on whether this whole confidence order thing is worthwhile is and is not huge, it just feels very clever. So we take those confidence orders. So we have a comment tree like A, B, C, D, and this is in place, Y to A. So each of these is gonna have a confidence order that's like three bytes. And I'm gonna do this in, bytes rather than the bitpack version, because I want it to actually be readable. So if we assumed that, let's see, it's going, I can't remember the order, is it highest first or lowest first? Order by... lowest first. So the confidence gets flipped from high values to low. So let's say A is a really upvoted comment. The maximally upvoted comment is going to have 00 for its confidence. And we'll say like 01. And this is at its level. So it's not any kind of, this is not a tree. So this could also be 00, a very highly upvoted comment. And this could be the same thing. And in fact, I suspect what's happening is the same thing. And then the comment ID, the low byte of the comment ID is a tiebreaker. And so, oh, yeah, that's actually, that's certainly what's happening here. Now I understand the bug. I don't know that I can write a test for it. I'm not sure I can explain it, but I understand the bug. So let's keep talking until I can actually say it, and then I'll know I really understand it, as opposed to I have an intuition. So we're pre-filling these values and 1308 does performance improvement by saying, well, let's just, instead of selecting out, instead of putting in 000 and immediately hitting the database to recalculate that, let's just grab that value and put that in. The problem is both A and B, when they haven't been voted on, they are both gonna have the placeholder value that is the same. And so they are going to look identical. And if they look identical, D, these comments are not getting sorted under their parents by, what is their parent ID? This is where that whole confidence order path thing comes in. So let's talk about what that is. It's this guy, which is some weird code. So again, what I'm trying to do is get out all of the comments in this tree order with the restriction that SQL really is not well suited to trees. so what i ended up with was this idea of a confidence order which is what is your place relative to your siblings and then a confidence order path which is well let's go ahead and say if we take the confidence order of all of your parents and we concatenate them together and then we put this comment on the end now we know your order in the tree and that is That's what becomes a confidence order path in, what's the name of that function? Story threads.
49:18So what's happening with these comments getting parented in the wrong place is,
They are getting, comment D is getting posted before A and B get voted on.
After A and B get voted on, they're gonna have a value that is based on their actual voting as opposed to this static value.
And then they look identical.
arh68 is it too much to concat the parent.short_id into the path?
And so then the comment sorts into the wrong place.
Probably the other thing I should talk through here to justify, is it too much to concat the parent dot short ID into the path?
Yes.
Yes, unfortunately it is.
This three byte thing was a really painful compromise with between uniqueness and width.
The
Confidence order path, so here for D, you can see it's six byte wide, right?
And so that tells you real easy that the COP width or length is three bytes times the depth of the comment.
And there's got to be a max somewhere, right?
Like what is the most replied comment?
And the maximum comment depth is, I made a constant for it.
I want to say it's 31 in production.
Yeah.
dzwdz is twitch being fucky for anyone else? i have to keep refreshing the page because the chat keeps breaking
So at one, the longest reply chain in production was 31 comments.
And so that is the maximum length of a COP.
But when I saw that, I was like, are you kidding me?
I can't tell anybody if Twitch is being weird and the chat is breaking.
That is beyond my purview.
arh68 site's fine for me so far
I can't even get Twitch to fix, well, bugs.
So while I was investigating this and figuring out the most nested comment reply chains,
I realized as I was looking at them that this one where people replied back and forth 30 comments deep, that it was actually a super unpleasant conversation.
And if I looked at the deepest comment threads, all of them were super unpleasant conversations.
arh68 LUL ya i tend to scroll past those right-hand-side discussions
Once you got past the depth of like 18 or something at that point, they were pretty much all people who were like nitpicking each other back and forth until one of them got frustrated and left.
And sometimes that was like left-left the site rather than just left the thread.
And so I capped max depth a little lower.
Right-hand side discussions.
Yeah, that's a funny way to put it.
That's also the other reason I put in a lower depth is as these comments indent, they get real skinny.
And also around 18 on any normal size display, it looked kind of ridiculous where, you know, each comment is...
two words wide, that was not great.
That's going to be a fun one for if and when I ever rebuild the site with CSS Grid.
dr3ig are you saying assigning precomputed values doesn't differentiate between parent comments prior to any upvotes ?
I've used it a little bit like the new-ish header that's maybe two or three years old that uses CSS Grid.
And I would like to start moving the page into CSS Grid because I came up and cut my teeth on all the float things.
Yes, Drake, I am saying that assigning pre-computed values doesn't differentiate between parent comments prior to upvotes.
That is exactly the problem.
dr3ig but then why did it work before
It is also the problem if we don't pre-compute because A and B, before they get upvoted, would have the same values.
Why did it work before?
53:45dpk0 morning/afternoon/evening all
That is a really good question.
...51arh68 morning dpk
because in practice, the idea of what's happening is we are inserting the same confidence order.
Hey, DPK, welcome back.
We're inserting the same confidence order values that would have been calculated.
Oh, it's the 255.
It's the 255.
So these two bytes, these are the real production values for a comment has exactly one vote from its submitter
well exactly one vote that is an upvote and no other votes in practice that is the one initial upvote from its submitter and nobody else has voted on it yet fine that is a very common value we were talking about like the median the median comment also gets voted on like once i want to say now close that terminal that's fine i'm not going to check it offhand
jameslittle230 gm dpk
So the tiebreaker when things have the same confidence order because, as you can imagine, the confidence order values for a comment with just the one vote from its submitter or.
the one submitters upvote and one person upvoted, or the one submitters wrote and two people upvoted.
Those are really, really common values.
hejihyuuga hello Mr. Pushcx, hello chat
Those are, you know, 30% of the comments in the database.
I'm just, I'm pulling that number out of my butt, but it's like directionally accurate.
hejihyuuga Hope you're all doing well
And so this third byte is, oh, welcome back Hedgie.
This third byte is,
the low order byte of the comments ID.
And that's a tiebreaker to differentiate comments with the same ID.
Oh, I didn't finish my thought here of, for ARH of, let's say four.
55:57And so ARH here was kind of suggesting, well, ARGH, you can tell which word I type more.
What if instead this was the several bytes of the short ID?
And we know offhand the short ID is six characters.
And so this would become, you know, five bytes wider or four bytes wider.
Could do that, but then the COP length
jameslittle230 Likewise Heji!
The max for the COP length, which is 31, let's say, then this would become 2 plus 5.
7 times 31 is what?
I have to do that with a calculator.
I don't know that math off the top of my head.
comes 217 bytes wide, and at some point when the confidence order path is wide enough, and that really comes around, because comment is already a fairly wide table, at some point it blows up the performance benefit of doing all of this confidence order stuff.
Because the row is so wide that the database has to allocate memory differently seems to be what's happening.
I haven't tried to dig under the covers of the query planner or the explains, but yeah, that's the verdict is unfortunately at some point COP becomes so wide
57:48Because the other thing to do would be to just put in the actual bytes of the comment ID. So we have, what is it?
58:02Yeah.
...14What is that?
Two to the...
mjiig 2^19 ish
12th offhand?
No.
16th?
It's getting in the neighborhood.
Yeah, there we go.
2 to the 19th.
I was like, we're right close to a value.
So 2 to the 19th.
So, like, 19 bits.
Thank you, MJ.
You are either better at remembering your powers of 2 or faster with the calculator than I am.
The...
byby42 id.to_s(2).size
Being only 19 bits wide would be nicer than being... What is this?
8 times 5, 40 bits wide?
But at the same point, we still run into this... We still hit this limitation that COP becomes too wide.
COP really can't be... Ah, ByeBye42 has a very smart version of it.
Yes.
Let's do that.
Didn't know you could say 2s with 200.
That's going to do a binary?
byby42 Hum
Oh, no, I can't do that.
Didn't LLM write that code?
byby42 Works on my machine
That would be nice if I could say 2s to just throw it over to binary, but there is probably a way to do it, you know, with string formatting and such.
byby42 Oh, id is a String here?
I don't want a rabbit hole there, but you are welcome to figure it out.
Chat works on your machine.
What does Ruby dash dash version say on your machine?
dzwdz exit \n python3 \n bin(2)
ID is a string here.
No, ID is the integer that comes
byby42 Integer#to_s(2) return the binary representation
back from the database oh i called the wrong i called it on the wrong thing nobody caught the bug i said comment.last not comment.last id ruby does indeed have that very nice and then yeah okay we rabbit hold anyways because i typoed i didn't say last id i just said last 2s that's okay so
01:00:36So my compromise for confidence order was, rather than being five bytes wide or three bytes wide, I just grabbed the low byte of the comments ID as the tiebreaker.
And it's kind of funny.
This isn't perfect.
And you're like, oh, like naively, there's a one in 256 chance that two comments get the same, have the same low order byte, except the site gets right around 200 comments a day.
And so it basically is significantly rarer than that because it would have to be that
a comment is posted and not voted on.
And then 26 hours later, when the ID has rolled around that bottom bite, someone comes back and leaves a sibling comment that also doesn't get voted on.
So in practice it is, it does happen, but it's very rare.
It's rare enough that I'm comfortable with it.
It's on order of maybe twice a year.
I think about once a year, somebody,
reports it but i see it about twice a year and it's automatically fixed by if i go and upvote one of the comments the bug disappears so you know it's very cheap i don't i don't love it maybe somebody looking at this goes oh there's a better way to do this confidence ordering thing but this is what i came up with especially given the limitations of sql where sql doesn't have
dzwdz if you order by c_o_p, the replies to a comment wll always come after the parent comment, right?
doesn't have a huge amount of bit packing functions that would do all of the clever stuff I want, especially not concatenating.
You have to kind of hit byte alignment and then turn these into strings.
Yeah.
So yeah, if I order by COP, the replies to a comment always come after the parent comment, right?
The problem here is 1308 says, well, we don't know the comment ID because we can't know that before the comment is inserted.
And so we are just going to insert a placeholder.
And we discussed this on stream of let's insert a placeholder byte of 255.
And we'll just put these last and so like when he comes in as a top level reply it'll come in last because presumably at some point A and B will get voted on and they'll have real values that are different.
dzwdz i wonder if you could do a pass over the returned data to try and force stuff into the right order
dzwdz since it's mostly in the proper order anyways
If they don't let's just tie break and we'll put you last what's happening, though, is now that all of these values are the same replies are showing up in the wrong place.
01:03:44Yeah.
So DZ, you're running into one of the constraints is the whole point of this big performance hack with confidence order and path is wanting to, on the Ruby side, not loop the comments.
Oh, and someone was saying, yeah, someone asked like, how many comments are we talking about?
What's the big O here?
dzwdz that was also me btw
There was a side point I wanted to make of, we looked at what the highest comment counts are and
The top one is 300 something and the next 20 are all around 200.
That's not a big N. What I'm trying to avoid is not long loops of tens of thousands of rows.
It is allocations.
Ruby, as a garbage collected language,
has to allocate stuff.
And we already did, you know, the whole guest star stream with by route about that ended up with boy, if we install J email, like we'll have less Ruby, less memory fragmentation, I was trying to reduce GC pressure, because if we loop the comments once, and we don't, that was also you, excuse me, not great with names, the
If we loop the comments once and we don't have to resort them and we don't have to build any data structure we get much less memory pressure on the Puma workers and in a Ruby web APP memory pressure is more significant than CPU pressure, we just have a smaller budget there.
And some of that is.
Ruby objects are very wide.
You cannot build a low overhead data structure.
I say that and I'm sure someone is like, well, if you write a C extension, yes.
We're not gonna write a C extension to sort 200 comments though.
01:05:47So the options here now I feel like I have a really good understanding of what is going wrong with 1308 and I would like to see if I can write us a test for it, because if I can write a test, then we can think about. Do we have to revert. Because I would like to not revert. The the benefit of 1308 is. This update score and recalculate runs this query, which is an update. And I did a lot of work to shove all of this work into the database, so it's just one update. But then the story has to run. And story update cache comments. Come here. Well, then that has to update the comments count. And then for all merge stories, it has to update those. And then it has to update the hotness with the calculated hotness. Oh, and then calculated hotness. Let's go query the tags out of the database. Let's query all of the comments out of the database. Let's hit those merge stories. And so it's like, okay, we thought we were doing one small thing. And instead we set off this whole cascade of various cache levels. This is also a couple of years ago, some folks asked for query information because they were writing their own database and, that ended up getting called Noria. And I think it ran for a couple of years and then got Aqua hired. If I remember correctly, someone would have to dig this out, but it was a fun bit of performance work. Somebody did with the lobsters database, cuz it has this kind of real world complexity. Because if you kind of cross your eyes, what's happening is we want to query out particular data, but it becomes expensive to query it out. And so we have layered on all of these ad hoc caches where comments count. That's a cache. And so we have to hit and update that cache. And hotness itself is also a cache. And all of these things become several intermediate layers of caching where the comments has a value and then that gets rolled up into other comments and it gets rolled up into story and into merge stories. And all of that is so that we do less selecting because the site is very, very read heavy like most sites. What if the... database was smarter about incremental calculations so that when you queried it realized most of the time the data hasn't changed and it managed all of those caches. wished the noria work could have made it into a popular database like say maria db where it could directly benefit us i wasn't quite willing to say boy the benefits are so great that i want to switch to a database run by a small team that's a little scary but the work they were doing like we can see all of this downstream stuff that are not downstream all of this This fundamental motivating work of we're maintaining caches at all these different layers and invalidating them and pre-filling them and ending up with duplicate values. And then nonsense happens. This is all just cache, cache, cache, and validation. You know, that's why the saying is the two hard things in computer science are naming things, cache and validation and off by one errors. We're seeing it right here. Yeah. Let's write the spec. inspect comments and just drop down to the end I don't think I yeah I might have created another describe block with that same name so I want to basically create this structure. I don't need C. I only need three comments to demonstrate the bug. It doesn't sort comments under the wrong appearance when they haven't been voted on. That's a very long name, and I will probably shorten it. get the first draft out. So we had comment A. And let's think a second, because in the spec, I always want to use these factories. And the factory, I don't think the factory is going to create different data. Let's look at it real quick. yeah it has these sequences and all this kind of stuff but it's not going to attempt to create the score or the confidence order path in a different way so we will be able to exercise the bug this way and these all need to be replies to the same story and i like to specify where i care about things so we will say that c is a reply to a and then so this is a this is just me expressing the bug a little and also if i write five lines of a test i want to see that it runs So this is like the source of the bug. So I'm not going to keep this expectation, but I want to get set up. So if I run it, green dot. No, undefined method, confidence order path. Oh, because it's confidence order. Confidence order path is a database specific thing where in the query that pulls out the threads, it generates the confidence order path. Yeah. Good. I closed the wrong terminal.
01:12:38So that's fine. I mean, it's not fine, it's the bug, but what I actually want to express is that if I say comment dot,
...56ban the spammer, if I say comment.story, was it called story threads? Let's just look at it.
01:13:12Story threads, yeah. For this story, we get this, and then I'm expecting,
...35What do I want to do? I really want to just check that the IDs are in the order. Well, I think I can just directly say this. I said sorted.toA to equal A, C, B. So this is what I want, but I think this spec is going to fail. Let's make sure it fails because... My expectation fails as opposed to I have to hassle around with arrays. That's very big. So we expected comment ID 1, 2, 3 to equal, that's not a, yeah. So that's correct, but it's not a usable diff. so let's just say sorted.map to grab the ids we expect to get a.id yeah good so now i have a spec for this bug which given it was a transient bug because by the time i so i was especially happy this person included a screenshot because by the time i loaded on it loaded it people had voted on various comments and i did not see the bug anymore because of course as soon as you vote on things the bug disappears which is part of why Excuse me, which is basically why the site hasn't been on fire the last week that this code has been in production. If comments were all showing up in the wrong places forever, that would be... That would be... I would have interrupted my vacation to drive to somewhere there was Wi-Fi. The... place i was at was so rural that the descriptions like one of the directions was turn off the paved road and then there are like 45 more minutes of driving so driving to get to wi-fi would have taken a second i had actually yeah i've been to this place a few times and starting a couple of years ago somebody built a cell tower and so you can kind of if you stand in exactly the right spot you can kind of get one bar it took literally i just set my phone down it took literally an hour to post the stream screenshot stream photo excuse me the stream photo to blue sky and mastodon like i hit post and just came back a lot later so there's our bug All right. All right. So this is happening.
01:16:52Initial.
...57This is happening because this doesn't know the actual comments id and so now dupes are super common what i wish i wish i could do an insert returning or Otherwise, instruct MariaDB, hey, I want you to fill this byte with this SQL expression. But I don't think I can express that in MariaDB, period. I know I can't express that through the layers of ActiveRecord. Let's go ahead and put that away.
01:17:59So the tempting thing is to say, well, the way 1308 worked was record initial upvote, which is the submitters. Who calls that? Who calls record initial upvote? I want to see that actually.
01:18:31Ah, the after create fires that hook.
...41And then it creates the vote for the user.
This is test code.
Occasionally we insert comments with different scores and flags.
This is just there for tests.
In practice, this never runs.
but then we have to hit the hash.
So really the cheapest, did I leave that edit?
Yeah, let's go ahead and get rid of that.
The cheapest thing I could do is say right here, touch the comments, confidence, order to insert the,
byte for the comment ID, comment ID.
And then I get to keep some of this benefit of number eight where I don't make quite so many queries back to the database.
And I'll have to touch, it'll break this spec because then that won't be correct anymore, but that's a pretty easy spec to fix.
And I feel like that might be the way rather than, let's think if I'm making a mistake.
So what I'm considering is, can I avoid reverting 1308?
So there are two paths, revert.
It's safe, it works, it's well-tested.
Don't revert.
It's faster.
It might help with deadlock.
And so I want to keep it.
If I don't revert, though, the downside is more clever special casing.
And I'm trying to think about, is this special casing worth it?
It really does save a whole bunch of round trips to the database.
Common insertion performance doesn't get my hackles up.
Even if it goes and does six database queries, the database is in the same data center.
It's very well sized for the amount of data in it.
And all those round trips are really just
even with the overhead of active record they're really just a couple tens of milliseconds which is not great like on one hand of yeah comment insertion feels like it should be the kind of endpoint that can run in 50 or 100 milliseconds in practice with rails like 80 milliseconds is kind of your floor so if we come in at like 150 to 250 that's not
The end of the world like that's a little slow but it's not.
it's not popular websites slow we're doing something like inserting a comment or placing an order takes 10 seconds, so I realized, I am kind of using a different scale than I would use in a commercial production.
code base where.
arh68 so in the SQL, the sort by has 1 field? like could ya sort by parent, (then) c_o_p i wonder
but there aren't dollars on the line.
There's just sort of pride on the line and getting comment insertion in 80 milliseconds instead of 150 is incredibly satisfying.
arh68 i think <1s is fine
So in the SQL, the sort by has one field, like you could sort by parent then COP, I wonder.
No, it has to sort by confidence order.
Where's, that's the,
the point of building up the confidence order path.
chamlis_ can you pick a random final byte, and get back to a 1/256 failure rate?
Yeah, I think under one second is like, it's fine for commercial work, but I care a little bit more about this.
And I like that, can I pick a random final byte and get back to a one out of 256 failure rate?
Chambliss, that's, so that's really tempting.
So I was briefly considering that.
It's either use, you know, pullback ID for final byte or insert random number.
The hassle with inserting a random number is it's not going to be a one out of 256 failure rate.
It's gonna be more like a one out of two failure rate.
Because the problem we're seeing is really this simple, that A and B have the same thing.
And C, which should be up here, is getting put under B.
And so, hey, get back into Vim.
And so if I inserted random bytes for A and B, it is a 50-50 as to which one of them is higher or lower than the other.
Oh, but then we don't actually care is A sorted higher than B.
No, it's not 50-50.
I'm thinking through it wrong.
dzwdz wouldn't random make this test fail 1/256
I'm thinking of sorting, not correctly parenting.
Yeah, we'd get back to 1 out of 256.
That is reasonable.
I was dismissing that too early.
I'm glad I talked through it.
Thank you.
01:24:55dzwdz as opposed to using the ids as it was done pre-#1308
Let's call it mis-parenting so I remember.
01:25:03Which is actually, this is the same failure rate as we already have. or no, this is higher than we already have because of all of those caveats I had about how slowly IDs roll over.
...24Yeah, so what would that look like? That's actually short enough to just write it. So if I said assign initial, I am gonna say, where's the attributes? I wanna have a,
...49And then here I'm going to say, what is it? Is it, I never remember the interface. Is it ran 256? All right.
01:26:06dzwdz i assume in this test the ids are sequential which is a pretty decent approximation of what really happens
And if I say like ran two a couple of times, all right, it is up to, but not including.
So that's exactly what I want.
So I am,
In this test, yes, the IDs are sequential.
And in the test data we saw go by, they were literally 1, 2, 3.
Because it resets the database between tests.
Or it resets the table, basically.
There are a few RSpec strategies.
So here...
...44So then this stays the same.
dpk0 stochastic programming: it could just be that you never got a โ2โ as a result from testing it out to find out if the argument to rand is inclusive or not, but you trust that the probability is low enough :P
And then...
the other spec from 1308, which was over in the bit packing spec, I believe.
Yeah, this thing becomes c dot
01:27:25chamlis_ I guess you could add one more byte from the ID/rand like you discussed earlier for a 1/65536 failure rate
Stochastic programming.
Yeah, DPK, I did like eight of them.
I'm not going to write a property test of the random number generator.
I was more confirming my intuition than I was using it for the first time there.
So spec model comment and spec model bitpacking.
Where did I put spec extras?
There we go.
01:28:00Yeah, if I add one more byte though, I'm starting to lose the performance.
And it's funny, I'm a little bit tempted.
There are really only...
chamlis_ ahh I must've missed we were right on the performance cliff
I don't think there's a depth on here, no.
So how do I wanna...
If I said...
I don't want to write the whole union.
Yeah.
Chamli.
Is it a French name?
Chamli?
The...
...48The cliff really is right there at about a bite or two. And so there are so few comments that go to a depth of more than 20 that... I am kind of tempted to... One way to get more bytes would be to say that the maximum COP length is not 31 times 3. It's like, well, what if that was 18 times 3, the same as the current thing? And the way I would get there is there really are only like 10 comment threads that go that deep i could just go and edit that data and like split those threads in the middle and leave a comment explaining like hey i edited this this doesn't match i rep i reparented a couple of these threads literally five or ten i guess i couldn't bring myself to edit the data just to get the depth to be able to have another bite like I don't know. It's funny. It's totally intuitive. I can't justify that one besides the vibes were wrong of editing data to get the performance that just felt a little too sleazy for me so did these i lost it all the conversation these specs did not run because oh the spec i fixed The spec of this specific bug didn't run. I expected at least that one would run. Let's go back. That's on confidence order path.
01:30:49Ah, it's not dash n, it's dash e.
Oh, it ran this time.
byby42 A bit late, but if I get some time in the future, I'd like to challenge the idea that sorting comments in Ruby would use noticeably more memory.
Really?
Is this like the it's failing one in 256 and that very first run I did?
Okay, well, now it's failing two out of three.
How many times do we want to run this spec to get an idea for it?
You're, yeah, a bit late for challenging the idea that sorting in Ruby uses noticeably more memory.
It's...
I think that's a pretty...
Hold on, I'm gonna clear my throat.
I'm gonna mute you all so you don't have to put up with that.
01:31:38byby42 Nah, don't respond, I'll just try to benchmark it at some point.
I think that is a really fair criticism of all of this confidence order thing.
How much actual performance gets actually saved by it?
If you wanna benchmark it, I would love that.
I have tried benchmarking it and it seemed like it was about a 20% improvement combining both the action ran faster and the GC ran less often.
And I say 20%, what am I talking about?
byby42 I think I can probably repurpose the YJIT-bench suite.
20% speed improvement to that end point, but then the GC pauses are distributed among
Every request that comes along, rather than just necessarily the ones that load a tree of comments so it's sort of hard to measure.
yeah you probably could repurpose the widget bench sweet that's that's clever that did not exist when I was doing this code so.
01:32:48So, Shamley, somehow my intuition that it was like we would be correct 50% of the time rather than 1 out of 256 seems correct because this test is passing like half the time, not 255 out of 256.
chamlis_ huh
So, we do not understand what's happening here.
This does not match my intuition at all.
Hmm.
I'm going to leave the code up and I'm going to run to the restroom.
I'll be back in about a minute or so.
Back in two minutes.
Yeah, if you want to puzzle out that bug, I'll see you in two minutes.
01:35:14espartapalma Hi, good morning y'all
Alright, hello again.
Anybody solve it?
Oh, hey again.
S part?
Yeah.
So, why does this test... Oh, there's a fun distraction.
I don't know if you all get the cute little animation that just played on chat, but...
nanerbag yep we did
espartapalma I got the animation!
I set a powers of two follower goal, and it just ticked over.
The first couple of follower goals Twitch had me set kind of blew through them immediately, which was really neat.
But doubling, they come much less often.
So how do I manage that?
Oh, you got the animation.
That's great.
I think those are cute.
How do I improve it?
So let's go ahead and say end goal.
You've ended your goal.
Now let's say edit.
So they recommend 270, but that is not a power of 2.
I don't know if I want to go straight to 512, because then I'm not going to see it for months.
What is a nice powers of 2 number bigger than 256?
Let's just add 32.
288.
That seems reasonable.
Then I'll see it again at some point in the next couple of months rather than, you know, two years from now.
All right.
So this test, this tweak of having an ID placeholder, I would have expected that C would start sorting into the correct place.
Oh, bye-bye, 42.
When you set up your benchmark, I was doing very ad hoc benchmarks where it's just kind of me watching the server logs and grapping out the production timings.
If you get a repeatable test suite going on that, I would love to see that code regardless of what numbers you produce because
byby42 Sure.
I could experiment a lot more confidently with this confidence order path stuff and try other approaches that just weren't worth 10 hours of implementation and then vibes based evaluation in production.
01:37:46Like one thing I suspect is if it's gonna be three bytes wide, where would I get if I did one byte for the confidence and then two bytes for the ID? Because comments really are clustered around, well, all the values that you would expect to see where it's combinations of one to five votes rather than, yeah, rather than using all of those two bytes.
01:38:28Before I set that up, though, I wanted to have some more test harnessing.
When I wrote that code, I ended up pulling production data to my local and then writing a script, which is probably still in the pull request or in the issue somewhere, maybe it's even in the repo, that sorted all of the comments on every story, printing out the tree,
chamlis_ maybe the final byte could be the current comment count mod 256, if that's available cheaply? would get determinism and slow wrapping within a story I think, though deletes might cause issues
then i took the existing code and i dumped that out to a text file and then i wrote this new confidence order implementation and then dumped out every you know the tree for every comment in every story and then i just dipped those two to make sure i was getting the exact same results in broad on either side so i
Did I edit this test wrong?
Where'd that come from?
All right, 255, this becomes...
01:39:33But that's not even the spec I'm looking at.
Maybe the final byte could be the current comment count 256 if that's available cheaply.
You need to get determinism and slow wrapping within a story.
Deletes are not an issue because we only ever soft delete comments.
However, to know the current comment count, mod 256, I would have to
go and hit the database and ask it how many comments there are, in which case I might as well just ask the comments ID.
There are the rails in production is a multi process layer called Puma workers, and then it's also multi threaded.
So it's not like I can just have a global variable and incremented every time a comment is posted.
It
it wouldn't cross the process boundary.
And that would also be even more layers of cache invalidation.
And so at that point, it just feels a little overkill.
So before I move on from this ID placeholder byte, I really do want to understand why is this test failing 50% of the time instead of one out of 256?
Because I really did expect that.
Oh, it's because there's two parent comments.
So it's failing for a different reason.
It's not the question of where does C get parented.
I bet if I looked at this, C would be under A one out of 256 times.
But if you ignore C, A and B are swapping positions.
So this is the 50-50 thing that I was expecting.
We go look at those diffs.
Yeah, so I expected 2, 1, 3, but got 3, 1, 2.
So 1 and 3, A and B are in the correct positions.
I'm sorry, A and C are in the correct positions, but B is swapping back and forth over A.
And that's harmless.
The test needs to account for it.
so so shameless how do we write this in the spec that we expect that we don't care if a and b are sorted differently what we're caring about is that c appears under a
chamlis_ sorry I've never written any ruby
Before I give up on this path, I would like to drive it out and see, do we get to a 1 in 256 failure rate?
Because then I feel confident I understand what's happening.
So I guess really what I'm expecting is that...
Sorry, you've never written any Ruby.
Oh, that's okay.
What I'm trying to say is that I expect...
So...
let's grab this yeah and then i am expecting that we have sort of a subset can i do that in ruby can i just say in an array because you can have arbitrarily nested arrays i want to say
without having to get into the indexes.
So if I have A as 1, 2, what I expect is something like 1, 3, 2.
01:43:26Well, I can say I expect either. Yeah, I don't want to expect either.
...46Is there a, I don't want to do each cons.
I don't want to, can I search?
Can I find a slice?
Because I don't want to make a slice.
I want to just search for, there is a slice of, you know, one comma three.
I don't care where it is.
And so there is,
find index, but there isn't a find an index of a slice.
byby42 Can't think of anything like that
And I am trying to keep this test reasonably expressive, which is why, if I said, if I have that in my A, there's a,
Maybe it would be reasonable to say Ada cycle.
Now, is it a cycle?
Oh, no, it's going to cycle a whole thing.
Yeah.
There is a way to say.
Chunk chunk.
01:45:19byby42 each_slice
I say, there's a way to say, give it to me in groups of two.
And at that point, I would get back an array that goes one, three, and then three, two, which is fine.
Each slice.
...46And then here I have to say each slice too. Yeah, it's not a sliding window. It's giving me it in groups of two. So that's not quite what I want. I want overlapping slices. Does that make sense?
01:46:11Where's zeal? Let's bring that back.
...23Yeah, see, this is disjoint. I want overlapping tuples. Can I just search here for tuple?
...39Sort by zip. Zip. That would be awful to do it with zip. Each cons which each here we go each cons calls the block with each successive overlapped all right. I don't know if this is actually going to make sense to anyone but me, but. So if I say each cons.
01:47:15byby42 oh TIL
And then I expect the relationships to include one a.id comma c.id.
That's, yeah.
So it's funny because Ruby at least allows me to express it succinctly.
But reading this, I know in like three months, I'm gonna be like, what on earth is each cons doing?
And I'm trying to express that.
01:48:30so i think now the test is going to be at a 1 in 256 success rate well so that's an improvement it looks like we're off the one half rate and now i feel like i'm getting immediately called back to my bayes method of at what point i'm building my own confidence over confidence order which is cute that this spec passes one in 256 times i'm not actually going to commit a spec that only succeeds one in 256 times but it would be nice to know so if i do the mental math what if i run this how many times do i run it to have to see a failure i think if i run it 256 times i'll have a some point it approaches e oh man my college math professors are mad at me right now especially because there isn't it there is the mathematical answer that it's going to involve e but then the practical answer is just run it 20 000 times and see that it fails about 20 or that it fails about 102 and 56. hmm
01:50:02dzwdz 63% chance of hitting a single failure i think
dzwdz in 256 runs
I don't actually want to sit here running this manually 256 times, obviously, let alone 2,560.
I don't want to write a script, so I am just going to... 63% chance of hitting a single failure.
dzwdz 1-pow(255/256, 256)
Yeah, I am sure there is a proper Bayesian answer and I don't know it offhand.
I feel at this point, I am sure
...34Oh, frequentist statistics. You're probably also right. So I feel comfortable enough that I understand what this code is doing to say yes, this, this spec is going to fail 101 and 256 times.
01:51:00And
I can do something evil to make this spec reliable.
So right now the spec is flaky.
chindiana_jones Vim is the best editor
And the evil thing I can do is... Let's justify it.
Set.
dzwdz (also, had to go afk for a while - i see we're just going with a random byte?)
necessary is one of those words i always struggle with hi chindiana jones that's a good title yes this is vim so if the spec is necessary flaky the generated ids can be the same byte so what i can just say is if
Yeah, an if in a test.
Who does that?
chindiana_jones What are you working on?
If a.id placeholder byte equals b.id.
Don't need to change fonts.
So I don't know that we're, DZ, I don't know that we're going with a random byte.
We are going with a spec.
01:52:33I just wanted to try the random byte and see if I could write a spec for it and just kind of feel it out.
So Chindiana Jones, I am working on we.
This is very much a we thing.
pushcx https://github.com/lobsters/lob…
dzwdz oh wow that if is cursed
We are working on lobsters, which is a web forum, and it has threaded comments, and SQL and threads don't mix.
Here's our scratch file.
I will just throw the main bug in here, and you'll have to follow links, but most of them are linked off.
We made a recent performance improvement in 13.0a.
That if is cursed.
Whoa, oh my god, Vim, what are you doing?
nanerbag that was new
I have never seen Vim.
chindiana_jones LOL
I pulled it off screen because God knows what it's doing, but it is scrolling continuously.
That's hilarious and awful.
I wonder if I did something to Alacrity.
Yeah, it seems to be looping the Alacrity buffer.
dzwdz vim's trying to stop you
So I am just going to blow that away and open a new Vim.
dzwdz good.
And let's get back to lobsters.
That was wild.
nanerbag ive only seen vim go wild when i open it inside a cli debugger
chamlis_ could you loop the spec until you can do an expect?
where's my scratch buffer so definitely something for that like what on earth did i do to alacrity to get it that was a weird one all right and i don't need a terminal i need a bin
01:54:19You've only seen Vim go wild when you open it inside a CLA debugger.
dzwdz so this test would pass on the current codebase
dzwdz i'm pretty sure
Yeah, I don't know if you saw this on the other stream, but since I use Vim as the terminal multiplexer, could I loop the spec until I can do an expect?
Yes, that is a different kind of cursed.
I think DZ is right.
This test would pass on the current code base.
I can check that.
I mean, with that if, well, yes, with that if it would pass because they're both going to be, I mean, it wouldn't pass because the ID placeholder byte doesn't exist.
But if we grabbed it, yes, they would both be 255.
So, yeah, I had to edit my Vim config to catch if I was opening a Vim inside of a Vim terminal.
Because vim x bucky, when you do that, it argues over key commands or grabbing various keys.
01:55:31Yeah.
dzwdz i'd maybe do a mix of the two approaches? run the test like 4 times, ensuring that if it fails it's for that reason
I mean, it is performance code.
You're allowed to be evil in performance code, right?
There's a stream title.
...50Do a mix to run the spec four times ensure that it fails for that reason that's.
01:56:01that's fair I don't want to over engineer this spec and I am aware, like this is heinous but.
...17but I am kind of comfortable that is the amount of heinosity I can commit. I don't know. So now that we've implemented, I think it was chamless, yeah, chamless. Now that we've implemented their strategy of just do a random byte and kind of looked at it, I mean, the benefit is it avoids that round trip to the database, which is the thing I, cause the alternative I had of pull back the ID from the final bite.
01:57:10dzwdz i wonder how the odds would change if you just added a bunch more comments
dzwdz wait no it'd just fail more
So what do we think this, this quality here is.
...18You wonder how the odds would change if I added a bunch more comments?
Yeah, yeah, it would just fail more because these are just random bytes.
So this is now, so let's think about what this means for production.
Because even with all the fun bit bashing kind of performance hacky stuff, we have to care.
Are the comments coming out in the right order?
dzwdz hm, if it would fail more with more comments
chamlis_ what's the birthday paradox number for 1/256 odds?
dzwdz it would fail more in production
And inserting a random byte means the likelihood of this exact bug, which has happened plenty, in addition to the four people here who have thumbs up, which I am taking to mean four people have seen this bug, I have gotten, I think, three direct messages from people reporting the bug to me.
What's the birthday paradox number for?
It would fail more in production.
Yeah, and it would fail more in production because comments can have multiple replies.
So it's not 1 in 256.
It is... Oh, and it's even worse than that because it is...
So here we have A and B and C. And C went to the wrong place.
But there is also D.
And so this goes wrong as a birthday paradox with the number of child comments that the two parent comments hit the same random byte for.
And every time there is a child comment, it is just flipping a coin as to which parent it's going to go under.
And so we are not getting it down to 1 in 256.
And I don't immediately know what that formula is going to be, but it is definitely not such a small number.
This feels like it's going to happen about once a day.
Because, you know, just kind of rule of thumbing, if there is roughly one byte of comments per day, and there are roughly 256, let's, here.
02:00:00I think it's date.
Date is the function to extract it from.
Yeah.
dzwdz i assume you're not using the last byte of the id because fetching it would be slow?
So if I say select a graded account, one from square.
We're in September, so let's say three months ago or six months ago.
Now let's just do the last couple of months.
...43yeah oh it's actually increased recently so we are running about one byte worth of comments hey look here's exactly 255 i mean it's not 256 but that that counts as a round number almost round-ish number so this really is going to happen way too frequently so chanlis i think i think this was a solid idea
dzwdz what if: two random byes
But now that we've seen it and kind of kicked the tires on it, I don't think I want to go with this.
This is...
dzwdz i mean it'd still be shit
It's clever, but it's just not quite reliable enough.
What if two random bytes?
Oh, two random bytes in the confidence order?
Because I could just override.
Oh, that's...
dzwdz no, i meant making the confidence order 4 bytes long
Well, then I'm, if I put random byte, if I make one of these two a random byte, we are definitely getting comments sorted wrong.
02:01:58dzwdz unless you can't
You meant making the confidence order four bytes long.
We are waiting on byebye 42.
byby42 Oh I didn't promise anything :)
doing a performance test harness before i'm willing to consider making that four bytes wide because if it's four bytes wide at that point i almost just want to make it five bytes wide and i know you didn't promise i'm just putting you on the spot i hope that
I was hoping that came across as teasing.
It was also a like, hey, are you still listening?
No, I know you didn't promise.
And also, it's a volunteer project, so even if you promised, I don't think we can really yell at you if you promise to give a gift and then don't give a gift.
Like, nobody gets yelled at for not giving a gift.
So... Yeah, alright.
Where am I at?
So I'm going to take this div.
Yeah.
02:03:20Can I just do this?
So there is something weird with my...
dzwdz possibly even worse idea: use the top byte of time()/60
My vim setup where I tell it to run a command and instead of just shells out instead of running the command and I know roughly where it is.
Oh, I know exactly where it is all right post stream yeah.
...53dzwdz it's basically sequential
Use the top byte of time divided by 60.
dzwdz also please don't do that i was joking
DZ, what's the value of that?
It's basically sequential.
02:04:10dzwdz er yes
I think instead of top byte, you mean low byte, right?
The rightmost digits of the number.
Yeah, yeah.
So we're big endian versus low endian here, but... Hmm.
I see where that gets.
Yeah, that's not... Let's just go ahead and do this.
Let's run.
And then...
I don't want to just totally throw away the code, so I'm just going to dump the... Oh, that's my...
What's the command to not use nox diff?
I want a proper patch.
And then typically I use difftastic.
So that's why it's getting that nice side-by-side view, but I want it to generate an actual patch.
dzwdz it wouldn't even make the test pass without the safety latch lol
So I'm not going to just, I am going to revert this code because I don't want to keep any of this, but I did just want to see it
02:05:27dzwdz unless you threw in a sleep in there
Yeah, it's been... Safety latch is a really, really flattering way of describing this terrible if.
Ah, safety latch.
Unless I threw a sleep in there?
Ah, it's just getting worse.
It's kind of fun to write that sort of evil code of, like, what's the worst way I could get this spec to pass?
02:06:01So I think the answer is going to have to be, let's insert the low byte of the ID to confidence order.
...19Oh, you know what? I think I actually do want this spec back. I shouldn't have thrown it away. That's the thing we're fixing. Good thing I saved that diff, huh? Every once in a while, I do something clever. And it's not too clever. Every once in a while, I mis-indent Ruby. So let's go ahead and drop all that. And we're going to get rid of, we'll keep that part of it.
02:07:03So I'll hang on to that spec for a minute. So what I want to say is, let's split this. Where's the, what I want to say is comment dot, no. self.update and this is where even having used rails forever i have to check this so there are a bunch of methods on active record models and one of them does a direct update that yeah bypasses validations callbacks are skipped And I can never memorize, is it update column? Is it update attribute? Is it singular? Is it plural? There is not a consistent way to do this. So we'll just say update column. Because again, I am allowed to do evil things in performance code. And we want to say update column confidence order. And I'm going to update it to Grab that, and then... And then the vote model. Yeah, vote models shouldn't have any... Yeah, there's no model hooks there, no callbacks, because that would be expensive. We get a lot of votes every day. Yeah, the other problem with cache invalidation, which is a lot of what's happening with all this story and vote stuff is it mixes really poorly with rails or active records fondness for model callbacks that happened before or after create. And at some point they just, it is very easy to have these cascade in a loop or like vote updates, comment, comment, updates, story, story, updates, vote, vote updates, and then you're off to the races, especially When those callbacks have ifs, that's been some very fun debugging for me at clients.
02:09:37So this, at the point that it calls record initial upvote, I think Rails automatically selects it back to have the ID rather than it still having the wrong thing. So if I said this wouldn't be nil,
02:10:01byby42 Not with `update_column` I don't think so
It's just, yeah, run that spec.
...12Wrong number of arguments given one expected two. What was update column? Name, oh, name comma value. It's not a hash.
...41byby42 update_column(name, val) / update_columns(name => val)
Can't write unknown attribute.
Oh, this needs to be a symbol.
...59Okay, so yes, Rails does auto-populate the ID. It didn't have to reload A to get its ID. And then the spec passed because the relationship is there. that gets us down to the very low error rate we expect.
02:11:21So while this is a round trip to the database, it is only one round trip to the database where always calling update score and recalculate, it's gonna kick all this off.
...41byby42 Yeah, still saves 7 queries IIRC.
And it might actually be, so the second thing I was thinking about, still saves seven queries.
Well, but yeah, you're correct that it saves seven queries.
However, it saves a query that I don't know we want to save.
I've been thinking about this 1308 and having traced it here at the start of the stream and then traced it again, this part is fine.
This is correct.
This is the long version of what I'm doing where we know that with only the submitter's initial upvotes that this is one.
We know this is zero.
So we know what these things are.
All of these are the same.
The only thing that is different is this ID at the end.
However, this calls update cached columns on story.
And update cached columns does actually matter.
So for example, we've been using that comments count column in our database.
byby42 :facepalm:
byby42 Totally missed that.
this immediately has the wrong number so now when a comment is inserted a story will not show that comment until it gets a vote or until well really until any comment in that story gets a vote that's not super and then
The other thing is each comment slightly contributes to a story's score.
Yeah, bye-bye, it's okay.
Biroot missed it and I missed it when I merged the PR, but I've been thinking about it and looking at it.
And I had initially said, well, yeah, so the cache is slightly out of date for a moment, but people vote on stories all the time.
but it feels like a pretty fundamental thing to get wrong.
The comments count.
02:14:16So I'm pondering whether it's worth it.
byby42 The comments count can be done atomically.
And one of these is gonna be the source of the deadlock.
Like we're touching comment and then we're touching story and then story touches comments back.
Well, but there's no votes in there.
byby42 But yes, I was trying to reduce the queries on story to reduce deadlocks.
comments count can be done atomically.
Yeah, yeah.
One thing I don't love about this is it's doing this on the Rails side where this could be a SQL command to tell the database to count rather than pull the count back to the Ruby and then.
Hmm.
byby42 Yes, but the issue is that to create a comment, you lock the story in the one transaction
byby42 That I believe is the source of deadlock.
yeah the deadlocks the deadlocks are something about voting and comment creation it is very rare that it's story creation which is kind of funny because story creation is way more expensive so it would hold that lock longer but creating stories only happens 10 to 30 times a day where comment creation happens you know 256 times a day hmm
02:15:38It's possible that the comment locking the story is the source of the deadlock.
...48Once or twice, I have traced, because on the Rails server output, you can see all of the comments that, or you can see all of the SQL queries that get executed, and I've kind of traced them for when I insert a comment versus when I change a comment. what's the path of tables we hit, and then the same thing for comment creation and story. And it didn't leap out at me that we were locking the same tables in different orders. It's possible it's there and I just missed it, but I didn't spot it immediately.
02:16:29byby42 To be clear I don't have strong evidence.
So what this really comes down to is
Am I okay with these caches being invalid for a little while?
...45Okay, I understand your clarification that you don't have strong evidence. Yeah, I don't have, I also don't have super confidence around the deadlock. It's one of those where it's so rare and fiddly that I don't try and speak too confidently around it. So what I'm wondering is... Is it okay that... So it's the comments count. It's the hotness. And then same again for merge stories.
02:17:48dzwdz i assume comments_count is the one displayed on the front page?
And merge stories are like, I don't know, 1% of stories, so that doesn't really keep me up at night.
But seeing comments count and hotness... Yeah, comments count is displayed in...
I believe list detail.
Yeah.
Yeah.
So list detail, this is the partial that gets used for the homepage, the list of stories that is used everywhere.
So it is basically everywhere the story title and that familiar row of links appears.
The comments count would be wrong.
02:18:42I guess what I'm asking is, what is the median period between a comment is posted and any comment on that story receives a vote?
Right?
Because if that window is very small, this is no big deal.
But if that window is
minutes long or hours long, we've inserted a bug.
dzwdz so the comment count only increments once someone updates the comment?
byby42 Story.increment_counter(:comments_count, comment.story_id)
dzwdz that feels bad for first comments on stories
The smaller version of it is, yeah.
02:19:44that feels bad for first comments on stories oh that is a really good point can i just come here twitch twitch give me that Struggling with the mouse cursor. I just wanted to just grab that straight into the log yeah that's that is especially bad for the first comment on stories because. Oh, and it's worse the the fewer comments on the stories are yet dizzy you got to a really good point here that.
02:20:26This window where the comment count and the hotness are out of date is going to be significantly longer for stories with few comments.
dzwdz and if you were going to calculate the median time you'd need to take that into account
When a story has 200 comments, well you know a vote is going to come in fairly soon.
dzwdz hm, how long has that been a thing for?
When it has zero or one, and it's especially the worst for the first comment because then the story will show zero comments and so people won't want to click in to read the first
comment because they won't know it's there that's brutal yeah so that's that is i think a really compelling point so i was writing down this question because like i can mentally start writing this query but i know it's going to take a couple of minutes to get right and then i was going to talk about hotness but this is so bad because
people won't click into stories with zero comments to vote and invalidate the cache and you know that points to oh what if we had a background job that every 30 seconds ran and poked all of the stories that are on the home page or on slash newest
And when I see that, it's just epicycles on epicycles trying to minimize the badness of the cache invalidation.
The other part is it just bugs me that hotness will be wrong for some period of time because that affects story scoring and it feels like it disincentivizes
posting a comment because comments contribute to story scores.
And so I'm kind of down on it for any amount of time.
I want stories to be able to move around.
I want the homepage to be dynamic as people are commenting.
And on stream a week ago, I was even talking about starting to theorize waiting comment points more because I think comments are
I think I called them the beating heart of the site.
Like they are the point of the site is to have discussions with other people.
And if we're not getting that very, very right to reward people for doing it and encourage good conversations, what are we doing?
So I think having seen these two approaches to implementation and thought about the consequences,
This is really just how I work on a typical basis of I kind of have to see the code to be able to reason about the consequences.
I think I have to revert 1308.
Because it's not just more special casing, it's
02:23:47It's invalid cache data on story and it's kind of. And for an indefinite amount of time. And I think that's really unpleasant.
02:24:04Yeah. So let's look at this diff. So this approach.
...16Let's look at the, yeah, patch. It's shorter on code than the random ID thing, but that's just a fluke.
...43So having done that, Having done that, I think we have to revert 1308.
...57arh68 revert seems reasonable, given all that's come to light
DZ, I'm not sure what you're asking about.
How long has what been a thing for?
1308 has been merged for about a week, 10 days.
Oh, that's the pagination.
I have two browsers open.
The other one was the pagination stuff, assuming we would get to that.
This turned out to be a much deeper rabbit hole than I expected.
So let's look at what all was in 1308 and see if there's anything to save.
Because it touched a whole bunch of code.
So this inserted placeholder values.
Oh, and it's not just this.
This is not the complete diff because I landed some code on top of this.
And I
Did I put it in the merge commit?
No, come here.
02:25:55Let's look at the recent commits.
02:26:09dr3ig pushcx, for my own sanity check: in order to build the comment tree of a story (if you were to build it in ruby), you only use the following data: comment_id, parent_id, number of votes, number of upvotes ?
So here was his.
arh68 # of flags ?
okay so yes i must have put it in the merge commit so it's showing here but not on the pr in order to build the commentary of a story you only use the following data id parent number of votes number one no the the other part you're missing is confidence that's why that term keeps coming up it's the where is it
...49Where's that function? Maybe you came in a minute late on the stream. I showed it right at the beginning. There's this comment hash number of calculate number of. We've been running long enough that I'm starting to run out of steam. So this calculated confidence is a formula for creating the difference between A comment that has a score of one because it has one upvote and no flags and a comment that has a score of one because it has 10 upvotes and nine flags. One of those is a better comment than the other. This function is a little bit overkill because on Reddit, people upvote and downvote comments. much more frequently because Reddit has downvote to mean I disagree or I dislike. And Lobsters, in part because we're a much smaller forum, basically got rid of downvotes. And there's the implementation detail that flags are implemented as a downvote. some of that is our historical path but it really is confidence not number of flags very few comments get flagged at all it has slowly moved into being a mod only signal
02:28:32It is not given where we go where the site has gone over the last dozen years.
arh68 i don't think i've ever flagged anything HahaHide
it's actually totally possible that confidence is not adding anything of value to the site anymore because.
In part, because we get fewer votes and that's just a popularity thing, but in part because.
02:29:08if people aren't downvoting, we're not getting that signal anyways. And so we're just doing a bunch of arithmetic to get to upvotes minus flags. So Drake, you know, the narrow answer is no, we need the comment ID, the parent ID and the confidence. But the broader answer is, Yeah, maybe it could just be the IDs and the score. Because score is, you know, upvotes minus flags. And if we're getting the same amount of signal out of it as all of this confidence math, but all of this confidence confidence math is not actually winning us anything? Do we want to drop that? Because then, then confidence order path could become something like score order path. And if it's still three bytes wide, it would have one byte that is the score, which, you know, or maybe call it like the score plus 10, comments can hit negative scores there is a limit there of negative 10 which is why I came up with 10 as a number and it is theoretically possible up votes are unbounded but for sorting we could bound it you know 255 or 245 and effectively not lose any data and then two bytes could be the ID of
02:30:56intrex111 hello hello
intrex111 how are you all?
How do I feel about that?
02:31:04Hi, intrex.
Welcome.
pushcx https://github.com/lobsters/lob…
intrex111 btw yelp filed a lawsuit against google
We are way deep into a coding stream.
I am not going to try and catch you up.
But the short version is there's this bug.
And in debating whether to revert the performance code that introduced it, do we want to just totally reduce comment scoring?
So does comment scoring, well, sorting, even benefit from the complicated, don't need to disparage it, from the confidence calculation?
Lobsters doesn't use downvoting much.
02:32:00intrex111 im still shit at all of this stuff, trying to get into uni rn to study this whole thing mainly cybersecurity
Wow.
Let's say it better.
Doesn't use downvoting.
...16dzwdz i wonder how hard it would be to make an userscript that sorts comments in this way
intrex111 what do you think about yelp and google though?
dzwdz to try it out
intrex111 okay okay
Intrex, that is neat, but we don't really talk much about businessy stuff on this stream or on the site.
intrex111 no worries
I have a personal opinion about Yelp and Google, but
intrex111 i meant the lawsuit btw
intrex111 where again sorry?
it's just not relevant this is a coding stream cool if you haven't seen hacker news they probably have a lot of responses about it and that site has open sign up so that would be a great place to go to talk about the lawsuit i am sure they have a thread on it and i will probably end up reading it this afternoon so
02:33:05dzwdz news.ycombinator.com
intrex111 there are a lot of noise around
dzwdz if i wrote it from memory correctly
Oh, Hacker News, it's, here, I'll give you the link in chat.
Ah, there we go.
Deezy got it.
...20intrex111 thank you both
Looks right to me.
If it links to a very tan and orange site, you got it.
...34intrex111 have a good day you two also be careful of telegram it aint end to end encryption
Does dropping confidence actually get us fewer round trips to the database?
Yes.
Yes.
dzwdz signal gang
Then 1308... Well, no, it still has to... Yeah, we're very aware that Telegram is not end-to-end encrypted.
It is a weird Russian social network, mostly used for...
dzwdz i use signal to cmmit all my scams and crimes
intrex111 there is a whole new case if you didnt know
nanerbag *Me who is switching between this stream and talking to my friend on telegram*
scams and crimes yeah man signal on a technical side i love signal they i don't i'm not a deep crypto guy i know a little bit about crypto like i can kind of read i understand what the double ratchet is doing even if i couldn't implement elliptic curve cryptography myself or even
intrex111 ceo got arrested and encryption being weird and stupid and all that
arh68 i am so out of the loop on all that stuff LUL
intrex111 i dont want to talk about it since you dont want to talk about that stuff
do any kind of cryptanalysis but like i've read some of their stuff and it the parts i get are very inspiring the organization is oobsters the organization is so weird and i'm still deeply skeptical after they introduced a pre-mined cryptocurrency i sort of understand how that happened where they're like oh people really want to integrate payments and it would be useful
But the cost of that was so I where they stopped updating the server code for something like nine months or a year and everybody was like hey has some lawsuit happened is something very bad happening we don't know and they just refuse to talk about it.
And then.
They introduced it.
It went really poorly.
dzwdz puts tinfoil hat on
My understanding is it has seen little to no adoption.
dzwdz the coin was a coverup for a lawsuit
There was weird price movements just before it was released and publicly announced that implied insider trading.
I don't know.
They never...
dzwdz to explain the lack of activity
If Signal did a retrospective about it, I would like them a lot more.
intrex111 if you want arh you can go on spotify there is a podcast called "security now" by steve gibson and they talk about all of that and more important stuff, they also have a link to a pdf which are their notes
dzwdz also they're lizardpeople
As it is now, everybody makes mistakes, but if they're not willing to look at those mistakes and learn from them and address some of the shortcomings,
I don't know.
The coin was cover up for a lawsuit.
Yeah, that's getting tinfoil.
Howdy.
But feel free to send that to me offline.
Yeah.
Steve Gibson.
I can see that that would be interesting if you're really interested in starting with security.
There are other podcasts for going a little deeper than Gibson does.
He does great intro stuff, but he is not a deep expert.
And some of his opinions are out of the mainstream of expert security analysis.
02:36:38intrex111 who would you recommend?
So coming back to confidence calculations, if we don't use downvoting,
Would this let us merge 1308?
Would this let us keep 1308?
Yeah, maybe.
Yeah.
02:37:0810, which is the, what is it called? Is it min comments score? Flaggable min score.
...29What are the most uploaded comments on lobsters? Now that I say all this?
...38Or is it substr? I'm never going to get this right.
...59dzwdz what about the comments_count?
on comments.storyid in the stories order by let's grab the score too let's look at the top 10
Oh, yeah.
There are comments that go way up to 200.
Not bad.
Not too surprising.
They're all going to be...
They're almost all on meta threads.
And then two or three spicy threads.
Yeah.
All right.
02:38:54Let's move that to its own line.
02:39:12Eliminates birthdays. I'm not remembering the right word. The reason I say eliminates is that scores are, Or cut stories are only open for comments for I think 90 days, and so, if we only get 256 comments a day. A 65 K ID is not going to roll over before the story. yeah. I think that might actually be a serious improvement. I guess really the open question is, how much does confidence differ? from score.
02:40:07Or another way to put it is if we store by score instead, how many comments change order?
Right?
dzwdz yeah i sent that ages ago
DZ, what about the comments count in what context?
...28The
dzwdz no np
Oh, you sent that ages ago.
Yeah, sorry.
It's hard to jump back and forth between chat, especially when I'm trying to think of everything I've said in the last 10 minutes to summarize.
So to answer this question, we could just dump out the comment trees again.
and diff them and then just kind of get some stats on it yeah that's that's pretty straightforward that's an interesting one it's a big enough change that especially absent this i kind of want to sleep on it rather than yolo it on the stream it's tempting to yolo it on the stream though i will say that because
It's fun to refactor this stuff, especially with commentary.
I've heard people talk about mob programming.
I guess this is my first time doing it.
02:41:44dzwdz i'm impressed just how much you've misinterpreted my question lmao
All right.
So let's go back.
I think regardless, I am going to revert 1308 here on stream rather than go deeper into
Oh, should we refactor the way comment sorting and confidence works entirely?
02:42:18dzwdz i asked it in the context of you talking about keeping #1308 in if you change the comment ordering
So this is the other stuff I was thinking.
Do we want to keep any of this?
And the answer is no.
Because this is just testing new behavior.
This one would pass anyways in production.
dzwdz because there's still the issue of the caches being invalid
But it doesn't really does it feel like it's a missing test from
...58Ah, DZ, thank you for explaining. Yeah. There's the cache invalidation. And we've mostly focused on comments count because it's really noticeable. Like, it's a very glaring issue of a story says it has zero comments and you click in and there's one. And especially if the timestamp is, you know, this comment was posted an hour ago, that feels really bad. It's very obviously bad. But the story hotness also, I didn't belabor it, but it really gets to me because I want the homepage to be dynamic and reflect people's contributions. And that's a very deep motivation. So increments correctly, that's all fine. We don't need that. Performance.
02:43:52I could almost keep this spec. And I say almost because it doesn't really get me anything, but it does express an invariant and leave open the door for can we improve this in the future? But the thing that it's missing is we don't have two more layers of expect of we expect story hotness to change. We expect story comments count to change. And that's why this spec was inefficient. So no, I don't think I want to rescue this. OK, so I'm just going to revert this entirely. What is the commit?
02:44:36All right.
I almost never run git revert.
arh68 tactical revision SeriousSloth
I usually end up keeping things.
Oh, I'm not allowed to revert a merge commit?
I want to actually see what dash m does because you cannot revert merge because you don't know which side should be the main line.
Oh, I see.
tactical revision yeah there's that question of if the build fails or rather the build passes and you deploy a site and it starts failing in production do you try to roll back or do you try and roll forward and especially after working at stripe on a enormous code base with huge volume my answer is roll forwards because
It's not just about scale.
It's a be honest with yourself.
Now you have possibly invalid data in your database.
You have all the customers who have that experience.
You are rolling forward, period.
arh68 can't roll back the clock
You cannot really roll back all the things that happened.
It is almost never the correct thing to do to toss the user's work in that way.
I have six.
Can't roll back the clock.
Yeah.
I mean, I almost don't want to say that because it's a little bit reductive.
Yes, of course, no one is saying that we have to be pure in some mathematical sense.
So let's trace these little ASCII arts.
I want this side of the merge, the 354 side.
Yeah.
I think this is literally the first time I've run git revert on a merge commit.
Oh, cool.
It doesn't have the parent.
I didn't follow the ASCII art.
So 4f6 has the parents 972 and 354.
Am I reading this wrong?
Let's take a look.
02:47:06354, 77, whatever.
...14Commit doesn't have the parent.
Git, yes, it does.
You just told me the parents.
I trusted you.
Who has run a git revert on a merge?
dzwdz lol
Because I am puzzled.
Why can I not?
...43dzwdz guess we're keeping #1308 in
Because this 972 side, that's definitely that last commit on the branch.
Yeah.
Guess we're keeping 1308 in.
Yeah.
Yeah.
Git itself has an opinion on what I commit.
All right, well, let's look at the, all else fails, read the docs.
Like, what are you, running a merge commit declares you will never want the tree changes brought in by the merge.
As a result, later merges will, oh, actually, maybe this is bad.
Later merges will only bring in tree changes introduced by commits that are not ancestors of the previous reverted merge.
This may or not be what you want.
I hate, I hate this sentence.
I have seen versions of this sentence.
I have written versions of this sentence and I always,
revert it or revise it because it doesn't express anything.
People always want different things.
You need to give me the criteria for deciding.
arh68 "it's just a DAG" lol this is the reality
And I realize some of that is this first two.
Reverting merge commit declares you'll never want the tree changes.
Maybe I just want to revert the individual commits, but I thought I landed a couple of changes.
It's just a DAG.
Yeah.
See the revert faulty merge how-to.
Is that a link down here?
Yeah.
All right, let's.
02:49:17Let's grab kit. Oh, yeah, I can't just.
...27You gave me a URL, it didn't work there. Oh, my God, no. So this doc of how to revert a faulty merge is just like, well, you're in trouble. Here is a mailing list message of Linus, Hunio, and someone else just riffing.
02:50:08I am tempted to just grab the diff of the merge. Merge would still exist. Yes, that's fine. I want to see it. Oh, future mergers will see that merge as the last shared state. So revert undoes the data, but it's very much not an undo in the sense it doesn't undo the fact.
...36Wait, why are we reverting a revert?
...57So I think it's warning that if I'm just doing a textual revert, it will still think those commits are merged to master.
And if a later merge comes along and says, okay, we're going to rescue those two commits off the branch and add one more, Git will go, cool.
Oh, I already merged those first two.
I'm not going to do it again.
So I think this is one of those, it is a problem in theory, but not in practice, where in theory, if I was going to reopen the 1308 branch and add another commit or two on top to fix it in some way, Git would get confused about what to actually apply.
But in practice,
We're not gonna reopen that branch.
It can just be dead forever.
We can redo the changes in a new branch.
And as long as by root knows that, and given that by root is, I think a rails contributor, they must've seen, he must've seen stuff like this before.
So we're just gonna go ahead and revert via diff, I think.
byby42 It's definitely no worries.
Yeah.
So if I say, let's show this, let's diff this against its head, right?
02:52:45Yeah. Let's say no x diff.
...55and then so if i ran this diff like this this is the inverse of the merge because you can see it's deleting the 255 placeholder which was the thing i added at the last moment and then inserting the old code and i'm just kind of rereading to make very sure i'm about to do what i think i'm about to do oh i am a little missing this this
Dot bytes that did make for a clearer test, but I would rather do that as a second commit because.
Yeah.
So if I take this diff.
And then I apply it.
One or two hunks failed.
dpk0 uh, since (afaict) you didnโt push the merge, why not just reset your HEAD to the pre-merge commit?
Because probably because we touched comment since then.
Yeah.
dpk0 or maybe you did push and i just donโt see it
dpk0 aaaah
so let's go look at the file and now assign initial confidence that's fine but the record initial dpk the problem is i did push the merge like 10 days ago so if we look at the git tree
I would scroll down a bit.
So 1308 was, who can spot it?
It's not a page down.
02:54:44Ah, yeah. So it was 10 days ago. Boy, why is it not jumping out at me? 1310. Ah, pre-compute. It didn't end up with the name in the title. So it was merged 10 days ago, and it was live for 10 days. And that's why there's all the bug reports, because we've had 10 days of comments being in the wrong place.
02:55:13So I believe that's what that was. Let's double check the diff. Yeah. We just always did it.
...33What was the comment? I want to put it back exactly. Trigger DV calculation.
...45Okay. And then how do I finish? Oh, it's just a patch failed. So the repo isn't in a weird state. So I can just remove the failed patch stuff. And if I look at this diff, this is now the inverse of the merge. yes so let's add all that oh and i can't commit because i'm in of him and of him there can i thought i allowed it nope so here let's just create it here let's revert i want that hash so Five, six, seven.
02:57:03Yeah, it feels like such a shame to lose that. I'm going to grab the one thing out of here that I did really like the. Bit packing part. Where. Down here. I'll go ahead and say bites bites.
...41So if I grab these spec changes, yeah. It was just so much nicer to express, especially to be able to say, like, I'm expecting this to be the ID, not some random hex value.
02:58:13and in this one it's going to be c.id except that needs parentheses and with that and then let standard rb take care of the spacing no didn't weird All right, and then can I run this full test? As long as everything is green. 000132. What am I? This is the place folder on creation, yes. All right, let's close that. Did I not revert correctly?
02:59:16Oh, it's because I am inserting the placeholder, but then the after create to recall record initial upvote is firing before it comes back to this spec. So how did this spec work?
...44I would think that this spec was failing. All right, hold on. Losing track of where I am. Let's back everything out. And Git shows no changes. Does this spec pass? No.
03:00:13All right, let's put everything back to the nice way now. So why did this run? Did I change the behavior of how record initial upvote works? Come here. Code on top. So this happens, and then this is left over. That's what's happening here. Okay. Yeah, I'm going to have to do a rebase here because that was dead code that shouldn't have been committed. let's see the whole suite pass.
03:01:06So that implies that I accidentally let that, or I accidentally committed the update code column.
dzwdz rebase?
I must not have cleaned up my repo before I ran that patch for revert, which means the previous commit has a line or two extra that I want to commit.
All right, so if the build is green,
Standard RB?
Rebase?
Yeah, I am going to do an interactive rebase to fix that.
I don't know why standard is only now catching those things.
And I'm not seeing the test change?
...58I am in a weird state.
03:02:12Yeah, this one came back, all right. I committed too much code. All right, let's go ahead and add this.
...27Let's look at the head. So this did accidentally include that.
...46did accidentally add this new test, which I am gonna leave because that's a good new test of the behavior. I don't wanna leave it in the revert. Oh, that's tacky. So let's go ahead and say, this is gonna be a fix up for the revert. And this is one of those times where I'm seriously tempted to spend a couple of days playing with jujitsu. It has a very different model to user interface than Git does. And I feel like I would have just foot gunned myself a little bit less here with that.
03:03:48So let'sโฆ This oneโฆ No, no. This is all standard. I don't know why standard didn't run on those. So this is theโฆ
03:04:17We'll reference that. And then these are just standard RV firing. So now I want to jump back to fix this. So we will go ahead and say, throw this code away. And the, I am sure that I could edit this, but the fastest way I know is to do a, to drop it and then make a merge commit. So we're going to do that.
03:05:08And then I'm going to put it back and
...22And what was this bug? 1313? Yep. All right.
...32So with that done, let's go with that away. I'm going to need the width.
...50Let's do some rebasing. So the rebase is happening on this bottom pane, if you can see that. So there's the revert, and we're going to fix it up, which is just not actually fix, it's squash. Yeah. Or fix up is the one that merges it in, merges the commits, and then drops the message. I don't know why that's in there, but that's fine. Oh, got a comment.
03:06:38Why do I have a conflict?
...45Yeah.
03:07:06just gonna just gonna be slamming my hand in a door over and over by adding and removing this code because it's gonna merge conflict every time i thought if i did it last i wouldn't but
...39All right. So now... Where's my spec?
03:08:00I think I managed to screw up the rebase and dropped that spec on the floor. I think this is an empty commit. Oh, no, okay, it's there. Don't sort when they haven't been voted on. What is this create in the middle doing? I must have... I must have... I didn't recognize it because I didn't go ABC. I must have accidentally edited it. Along the way, some... The one hassle with Vim is random typos can just do substantial edits to things.
...42Why did I do that? So just checking amend.
03:09:04arh68 does it complain like go if ya don't use `b`
arh68 idk otherwise
All right.
All right, so here's a whole pile of commits.
We're gonna look at them to make sure I'm actually
committing what I think I'm committing.
So this is the revert of the merge, and there is not the extra line of code.
There is the tests, changes back to what the specs were before the merge.
That's all fine.
Yep, that was a new spec.
That's all good.
All right.
And then this one is just the improvement to the spec.
And this one is just standard RV that should not even be firing because I haven't touched these specs in ages.
Maybe they decided a different rule.
All right.
So I expect to have a green build here.
And one of the ways I am going to be a little bit sloppy is I'm not going to make sure that each of those different commits has a green build, especially because I assume that the standard RB is breaking the build on the previous ones.
All right, so if that's all green, that's good enough for me.
Let's push all that up.
And then with that pushed,
03:10:31We're going to, I'm going to bring in my personal browser, which is over here. There we go. So all of this was to investigate this bug and here's people calling it out. Yep.
03:11:05let's name that commit it is i don't need a so
03:12:04I'm just going to predict, because I know what URL works here, even though it won't have a title on it. And this is a broken link. But when I end the stream, this will be live in a couple of minutes.
...55dzwdz fyi i'm writing a quick and dirty python script to see how ordering the comments by score would change the order
dzwdz (although i think that even if the order changes it won't be a big deal)
dzwdz i mean i'm just curious
I don't think you have to write a quick and dirty Python commit.
Give me a second to finish this commit message.
Or if you dig around in the Git history, I'm pretty sure I committed my script for dumping all of those out.
And so you could just have a running start on that is what I'm saying.
03:14:10New paragraph.
...57I'm trying to think of the right way to say this. It's not that it... It's just a little harsh to say it caused a bunch of problems.
03:15:45What's the O method where it's not like the command pattern, but a service object?
03:16:21Lifecycle callbacks.
...54OK. Yeah, I think that's the best way I can describe that.
So with that done, I'm going to go ahead and started deploy.
dr3ig you said at the start of stream you were planning to rework stories and comments models; do you know how you want to go about it, more or less?
And.
And DZ, let me see if I can find that code for you while the deploy runs.
I said at the start of the stream, I was planning to rework story and comments models.
Do you know how you want it to go about, more or less?
What's different here?
Dang it.
Did I really?
I think I pushed a broken build.
Hang on.
or this git is just out of date.
Hold on, let's close that.
Reopen it.
You still say there's a change?
dzwdz i love how i only realize chat has broken for me once you start interacting with someone else
Oh, I'm in the, why did I not get that in there?
dzwdz i get dropped for a while, then i "reconnect"
Oh, it's, all right.
All right, so the commit history was correct.
Well, I must've like rewritten a local buffer or something.
Oh boy.
Oh, DZ, I'm sorry that you're having that chat issue.
That sounds really frustrating.
dzwdz i should just connect to here from weechat
Yeah.
So the specific reworking I wanted to do.
Oh, let's get Ansible running again because it's not actually broken.
Connect from WeChat.
Yeah.
03:19:08So Drake and Drake, perfect timing for this question because as I'm wrapping up with a deploy of the work that got done here on stream, it is a great time for anybody else who has questions or comments or queries they would want to run on the production database.
Now is a great, great time to ask them because I can drop all of the code out of my head and we can go ahead and work on
office hours kind of stuff.
So as you look at the story model, the way merge stories work fundamentally is the stories table has a column called merge into story ID.
So if these stories, if a story has any stories merged into it, there are just other
story records and so the problem is the idea of being a merged story is whether or not anything appears in this column merged into story id and it is an infinite source of bugs where almost always what we want to be printing is the title of the merge target and not
The title of the story that the comment is on but it's much more natural to write that, of course, and I think what needs to happen here is a refactor to.
separate out a story object that has a title a short ID.
Probably votes.
And most of these associations and everything move over to a something else object.
Call it maybe, I just used link for another table, but call it like a URL because story is a URL and or a text.
And then all of those have a foreign key to story and always has at least one.
And so then it's,
It's breaking the idea that a merge story is special structurally.
And so every story that is just one link, so that common case, you know, the 95% case, that would still have a story record and then one of these individual objects.
And I want to say I left a comment talking about this a bit in the issues.
pushcx https://github.com/lobsters/lob…
fairly recently yeah this is so as you can see by the fact that there are immediately three things just this database structure infinitely promotes generates bugs
yeah so here's the comment i was thinking of so if i copy the link i can drop that to you and it is a slightly shorter way of saying what i said slightly better but drake does that make sense of what i'm trying to get at is the database model needs to to better reflect
dr3ig yes, makes sense, thanks
story merging as opposed to it sort of bolting on and this idea of does a story have multiple links or texts merged into it it's kind of an endless source of bugs the other thing that could happen is that would prompt remodeling the story page a bit okay good i'm glad that makes sense the other thing is
What was the title of the one that I went to that had all the merges?
It was something about XC.
There it is.
So we have this view of story merging and it has two issues with it.
Number one is when you look at this on the homepage or anything that displays stories, you see this line and you see this line and you see no indication that
eight or 10 stories have been merged into it.
It would be nice to add, you know, something that counts the number of links into it in that list detail.
So the other way to say it is there is no way to look at this and know if any of these stories currently on the homepage have things merged into them.
Oh, and I'm going to have to merge these two stories because that's a dupe.
Topical.
And another one of these Rust Linux thing.
Well, let's see if that's been a week.
Sparks, is this a Mastodon?
All right.
At least it's not a Mastodon post.
So it would be good if this indicated that there were more stories merged in.
And then in here, as comments get merged in,
they get an icon to show that they were merged.
Oh boy, and it's gonna be hard to find any.
Here we go.
So this one was not on the primary link.
I think it would be really good if we had a section heading in here that was the alternate link that this is specifically responding to.
And so a story would have multiple links and then each of the alternate link, each link would have a heading in here very similar to its list detail so that that was clear, but then all of these appear on one page.
And so we keep our discussion together and we cut down on the amount of rehashing we do.
There are also some other nice things like slash active could show the latest story.
the latest link merged into things.
A frustration of this, aside from this is a big block, is people who submit responses, especially when they are the author of the responses, are frustrated by story merging because they end up feeling that their link was denied the attention it deserved, or maybe even, to be more charitable, deserved
Was denied the chance at attention that it deserved by being a top level story because, again, only this first one appears for story merging and if the database structure better reflects that a story is multiple links.
We can do things like have a page that shows the latest submitted links, regardless of what story they're merged into or.
I hesitate to offer an option, and I definitely won't offer an option to never show story mergings and split them out, because that breaks the value to discussions of not repeating stuff.
But it would be very nice to maybe have an option that lets you see the latest link on a story instead of just the newest, because then readers would see
constantly rotating list of links on the home page where you would see the last one down here I don't know that might not be a good option but once the database model is cleaned up we can kind of kick the tires on it and design a few different things because the database supports it much nicer so that's something I've been chewing on for oh god maybe
maybe about 15 minutes after I became the admin and started having to fix all of the bugs related to story merging.
And I tease a little, but it really does generate a new bug, maybe two or three bugs a year.
And I realize in a commercial setting where someone's getting paid to work on this 40 hours a week, that's not a hell of a lot.
dr3ig would the comments belong_to a link then instead of a story ?
dzwdz unless i messed up something in the script, only 4 comments in the largest thread moved: https://pastebin.com/kdANS5eU
But for us as all volunteer with a fairly low pace of change, that's a lot of bugs.
dzwdz (diff between old and new order)
So that's why that's hanging around and.
If you are asking because you are thinking of doing it, I would love to work with you on that.
03:28:02Unless you mess something up in your script, only four comments in the large thread move. Oh, I was going to look around and see if I had that. Script in the. Still in the repo.
...21Something about threading? No. Is it in extras? It's possible I just did it and threw the script away, but I thought I committed it. I may not have kept it. I would have to dig back and find the confidence order commit. Only four comments in the largest thread moved.
03:29:07dzwdz for the record i'm assuming that the json output is in the same order as the comments on the site
dr3ig no, i was just curious; it sounds too scary to take on; i'd love to watch you stream it though :)
dzwdz which hopefully isn't an incorrect assumption
dzwdz yo free giftcards
whatever this stuff is.
Oh, I see.
So you wrote a script that went and hit production, and then you took the current order and replaced it with just the score?
That's clever.
dzwdz one :)
DZ, how many threads did you run this on?
And yes, the JSON output is in the same order as the comments on the site.
Just the one thread.
Yeah.
...49dzwdz i'll see the xz one, i bet that one had more flags
So I see this moves like three or four threads around.
...59Oh,
This is the homepage review thread, isn't it?
Yeah.
Yeah, the XZ1 probably had more flags.
Anytime something real spicy happens, there's more flagging.
Okay.
The other thing is, this is also a judgment call of, is the ordering better?
because the ordering is different, but it is not necessarily worse.
Not every change is bad.
dzwdz i don't think it matters on the scale of a small comment thread
And just doing score directly might do a better job of shoving down the things that have been flagged.
dzwdz tbh
It might not.
I honestly don't know.
Or maybe something simple like changing the weighting of flags so they're minus two instead of one.
Yeah, I don't know.
In the scale of a small comment thread, it almost certainly doesn't matter because if there's three or four comments, you just see them all.
dzwdz er, i did not mean to put the "small" in there
It doesn't really change anything if they're in a slightly different order.
If you want to see a really big change, there is one feature I would like to steal from Hacker News.
dzwdz overall i meant that comment threads are small enough for this not to matter
It's a big change to one of the things that they do
is on very new stories.
And it might be one hour.
It might be the first day or so.
Overall comment threads are small enough or not.
Yeah, probably roughly.
So we score comments where we just try and put the highest voted thing first.
And a downside of that is a bunch of people click into the comments.
They read a couple of comments and then they leave.
but also they interact with a couple of comments.
So there's a rich get richer effect where whatever is uploaded early on gets all of the attention.
So Hacker News does a clever thing where if you watch these timestamps, you can't see the scores, but they show threads where the top level comment, especially this first one is actually a very recent comment.
dzwdz oh, cool
gsvolt i'm traditional - always prefer chronological comments - regardless of how many votes
And then there is the most uploaded comment.
And then there is the second newest comment.
arh68 oh does lobsters not do that already ?
and then the second most upvoted comments, and they kind of weave them together so that comments that are brand new show up interspersed in the top couple of comments so that they get some attention.
And they're doing, they always have very low vote counts.
And no, ARH, we do not do that already.
That is, I think HN only started doing it
five or six years ago.
At least that's when it came to my awareness.
I picked up on what it was doing.
But Lobster has never done anything like that, and that is a feature I would love to steal.
So if you want to add one more complexity onto doing something with confidence order, that would be pretty neat.
And again,
It is possible that this is just me having an old attitude of I want to shove all of this stuff into the database and I want to get things, I want to make the database do the sorting and I want to loop it exactly once on the front end.
And it's possible it's actually totally fine to do something like, well, pull it in the confidence order from the database
but then walk it once to find all the top level comments sorted by time and then maintain a second list.
Yeah, GSVolt, we actually, as a point of opinion, don't allow you to just sort comments by newest and always present a tree.
I don't know.
Features, features everywhere.
there are lots and lots of things we can continue tinkering with.
arh68 can I ask a couple random questions
So I hope that was a fairly interesting stream of deep diving on a performance improvement that unfortunately turned out to be a little buggy.
And a lot of why does the site do some of the things the way it does?
arh68 when you were lookin at that merged story, how do those avatars show? do i have to turn those on
And most of it is
arh68 nah like USer avatars
what leads to good discussions and then a small amount of it is let's just tune the hell out of performance because you know some amount of that is fun and some of that is very valuable how do those avatars show oh you mean these merge icons i think there might be an option on the settings page to hide these on comments i don't remember if that's behind a flag
Oh, the user avatars?
Yeah, that's on the settings page.
You can turn those off if you don't like seeing those, you want to save the data, or you are using links.
arh68 oh i guess i never turned that on
It's funny, a couple of the options we have on the sites, like hide avatars, are very, very specifically, we have a bunch of gray-beard folks with a bunch of very strong opinions about
dzwdz wait does comments[].score include the flags
dzwdz if so then my code is broken
not sure about these images on my website i get a kick out of that all right so i'm going to go ahead and unless there are any last minute questions i'm going to go ahead and end the stream because we've covered a lot of ground we wrote some code we deployed some code things are in a better space hopefully a bad bug is fixed does the comment score include the flags so score is
Excuse me, score is defined as upvotes minus flags.
arh68 when I pull up a never-seen-before thread, is it supposed to say 0 unread ? i'm ok with it, jsut strange
If so, then your code is broken.
Hey, congrats, you wrote a bug too.
Everybody gets a bug.
You get a bug, you get a bug.
If you want to play with that and revise, go ahead.
When you pull up a never before seen thread, is it supposed to say zero on red?
No.
dzwdz does that happen with comments on there?
it should say nothing that sounds like a bug it's not nice to nerds might snipe me with bugs that i wrote just before i end the stream so on story show i put an or zero
Oh, this is a bug.
Yeah, it should be a... Let's just grab it.
It shouldn't be possible to see zero.
You see this if?
03:37:25This if is saying, if there is an unread count greater than zero, include the unread at the top.
I believe you're talking about the...
arh68 i see 28 comments, 0 unread on like m9bhxk. yes
i can't see it because i'm not logged in but you're talking about the display that appears like right here between the list detail and the threads where it says there's a little summary for logged in users and it says you know 306 comments 14 on red yeah arh that is a bug and i am not immediately spotting it we're saying
dzwdz surprisingly more comments moved around once i fixed the flags counting twice
if the unread count is greater than zero, then we display how many are unread?
Oh, it's the else.
03:38:16So I don't think it should say, I mean, at this point, what we're explaining is zero read.
...34dzwdz https://pastebin.com/1KvFs6Wv fixed on xz
so really what it's saying is all of the so this is a database implementation detail so the way that unread works is this read ribbon guy which is the timestamp of the last time you loaded the page and if you haven't loaded the page before you don't have a read ribbon
And so it's saying they are none unread, which is not correct.
It just says you haven't loaded the page before.
So it's this else message is wrong.
So really we wanna say, we wanna have one if, or if there is a ribbon, we're gonna do something.
And if there isn't a ribbon, we'll just say all on red.
dzwdz actually i think it's still wrong. whatever
And it doesn't have to be red or highlighted because they're not going to have the little red links on the comments because there isn't a read ribbon.
And then, yeah, DZ is really hard to live code and iterate on stuff and maintain a conversation.
I end up with writing significantly bugier code.
And I think you are enjoying that same experience.
That's OK.
I appreciate that you're trying to hack this stuff out live.
It's a lot of fun to do it interactively.
So thank you.
So if there is a ribbon, and then
03:40:35if that ribbon and we can't this is where i need the or zero because it's possible this is no it's not possible but this is nil anymore so now if it's greater than zero print that otherwise we print zero on red yeah this is a okay
man that's now we refactor so we'll just grab that and we'll say if there isn't anything you get all unread get that
arh68 missing trailing %>
gsvolt eh, elsif end tag needs a percent symbol I think
just kind of flatten this out i really don't want to have a nested if and then this does that look correct where we're saying let's try and grab their value if they don't have one they are all on red this is also going to fire if the user is logged in or if there is no
arh68 looks good otherwise SeemsGood
yeah i wasn't thinking about the syntax just yet but i see it so i think i want to say have a separate one here where i say if not user just do nothing oh i already have a if user up here okay so we're not going to have to worry about that branch
03:42:36dzwdz or no, i think it's the correct, i just got confused by the diff
house and right all right so if we have a user and there are any comments then you get a summary with the number of comments and if you don't have a ribbon
We say that they're all on red.
And if the number is greater than zero, we do the nice clickable red guy.
And otherwise, we say zero.
And the only difference between these two is the styling.
And I would rather just express it like this.
Why do I have this raw?
I can get rid of that, right?
At this point, it's refactored to the point that I can just say this.
Yeah, that is a weird use of raw that was never necessary.
Let's be a little safer, I guess.
dzwdz the current way comments are sorted causes weird stuff in the xz thread - there's a -10 deleted comment that still manages to be above 8 normal comments
No, no XSS injection in your unread count.
I like that.
Let's see if the specs pass.
Either I have a typo and half the specs are gonna blow up, or I don't and they're all gonna be green, because I don't think I have a spec for the summary.
03:44:18The current way comments are sorted causes weird stuff.
There's a minus 10 deleted that still manages to be above eight normal comments.
DZ, that sounds like a bug.
dzwdz yup
If you're saying,
Is the minus 10 comment that's deleted, is that a top level reply?
I think I remember deleting that one.
If it's still above other comments, that's just straight up a bug.
Come here.
dzwdz yup
since i think since i'm logged out in this browser i may not be able to even see the little tombstone i don't remember comment removed by author author by author are you referring to this comment here yeah logged out so i'm not
dzwdz i don't see the flag count either but i got it in the json
dzwdz hmm
dzwdz er, the score
i'm not pulling up my logged in user view because this stuff gets decorated with moderator only information like i can see the names of people who have flagged comments so i'm not pulling that up on stream and now i'm seeing the logged out view that doesn't include stuff like that's boy that really feels like a bug there is no way that the confidence on a
You don't see the flag count either, but got it in the JSON.
That's probably supposed to be there in the JSON.
Yeah, that's fine.
So this feels very much like a bug.
Like we've caught two bugs here at the end of the stream, and I'm okay with fixing this one on the stream, but that other one is complicated.
Why is this...
Someone hacked my box and keep reverting code?
Or am I just...
Goofing up.
Reverting that bad edit.
Okay.
So that's fine.
03:47:08There we go.
Yeah, that's a good commit message.
Okay.
I'm going to get that deployment started.
Okay.
arh68 cool. HahaCat that was quick
That's a thing I don't want.
And...
Let's see.
Okay.
...59I'm going to update that guy in a second.
This part is fine.
Yeah.
All right.
ARH, thank you for catching the bug and reporting it.
It makes it very easy to make those kinds of improvements.
We really did a deep dive on Rails and database performance and all that kind of stuff.
So it's really satisfying to have the happy path of, oh, yeah, on Rails, you can just knock out a feature in three lines of code and improve things.
Kind of funny, the dichotomy.
At least the easy stuff is generally easy.
All right.
So DZ, you're seeing weird sorting in a live thread that is just, that is straight up a bug if a comment with 10 flags on it is scored above.
I can kind of imagine.
I would have to look at that in the database and see, you know, did 50 people upvote it and only 10 flag?
No, then it would be it.
arh68 is there like a local dev mode where you can dump [x, y, z] into the markup
I don't know what the net score on that comment was.
I don't remember what the specific wording was.
Occasionally somebody posts something mean, but insightful in a mean way, and people upvote it before I end up seeing it to remove, but I'm not sure.
arh68 i'm still kinda foggy on those confidence score things
Is there a local dev mode where you can dump XYZ in the markup?
Yes.
I could just say something like... And then...
Debug prints out whatever that value is inside of pretags.
It's just a handy thing.
You can also drop into a debugger.
I almost never use one, but some people swear by them.
03:50:01dzwdz wow automod hates me
dzwdz is debug a builtin rails thing?
dzwdz lmao
arh68 PogChamp
erich if you're oh how funny so dz just tweaked automod yeah so dz asked is debug a built-in rails thing and i guess that automod has a dirty mind because it said that that was sexual content i can kind of see how you get there but
So debug is a built-in rails feature.
And then by bug is a gem that for an interactive debugger that almost everybody adds, but yes, it's a built-in rails feature and auto mod is very prudish.
Okay.
I am going to go spend a minute looking at that comment with the terrible score.
Let's copy that link.
Oh, it's funny.
I have this whole thing of knocking stuff off stream, but it is physically on the same monitor.
So I've made a bunch of changes to the window manager.
So I can't drag windows onto the stream accidentally, but I guess menus can overlap.
Yeah.
So if I open like a calculator,
I have to hit a hot key to say, oh, yeah, this calculator can come on stream, this floating window.
And if I say it can't, the window manager immediately shoves it off.
It never gets one frame to render, which is kind of handy.
dzwdz ruby rails us all
But physically, the off-screen browsers that are not being streamed are below the streaming area, and so their menus can overlap.
So you saw that for a second.
That's kind of funny.
All right.
Ruby rails us all.
Yeah, that's please do not taunt happy fun auto mod.
Eventually it will bite you.
All right.
Take care, folks.
Have a good one.
arh68 cheers y'all HahaGingercat
dzwdz this was an interesting bug
chamlis_ thanks!
I will see you back on next Monday at 2 p.m. as usual because I will not be out in the forest.
And maybe then we will pick up pagination.
Maybe ARH will catch some more bugs or DZ will...
dzwdz the c_o_p one
prove that comments should be ordered very differently, and we'll do that instead, but we'll see how it goes.
Shamless, thank you again for your suggestion of the insert a random number.
It was a good idea.
I'm glad we worked through it to figure out what was going on there.
It was a shame it didn't work out.
And yeah, the confidence ordering path was a weird one because the code is so clever.
I get more and more suspicious of clever code the older I get, but, you know, performance and our big primary thing is loading a story, reading the comments on it.
So that is the one path where I will do ridiculous stuff to improve that performance because we serve a lot of those pages.
I mean, if you want to see the kinds of depths I will sync to, you can look up heinous inline partial.
It occasionally gets mentioned because I put a big warning for it that happens every time Rails loads, but that is the sort of awful thing I'll do for performance on the one hot path for the app.
All right.
dzwdz do you have any idea off the top of your head how you'd do the hn comment interleaving thing with c_o_p?
Maybe one of these days I will explain it on stream and we'll actually investigate it and someone will point out that it's redundant or I will foot gun myself with it on stream and we'll have to talk about it, but not today at least.
No, I have no idea how to do the comment interleaving thing with confidence order path DZ.
That's part of why it hasn't been done yet.
And it may just fundamentally be something that cannot work with confidence order path, or it may require...
only thing that came to mind was it's possible that you could layer on a union query and you have a union where you say give me all the comments in confidence order path union the ones the newest you know call it the 10 newest top level comments that don't have replies that don't have three votes on them and then
order by and then do something brilliant in that order by that's that's kind of my best guess but i can't write it in my head so i don't really know what that's going to look like i don't know maybe there's something there maybe it points to confidence order path while clever is just too clever and it's not going to work out and we should take a fundamentally different approach i don't know we'll see how it goes always learning
Thanks for hanging out.
Take care, folks.