Hi, On Tue, 24 Jun 2025 at 18:12, Melanie Plageman <melanieplage...@gmail.com> wrote: > > On Tue, Jun 24, 2025 at 9:17 AM Melanie Plageman > <melanieplage...@gmail.com> wrote: > > > > On Tue, Jun 24, 2025 at 4:01 AM Nazir Bilal Yavuz <byavu...@gmail.com> > > wrote: > > > > > I think we do not need to check visibility of the page here, as we > > > already know that page was not all-visible due to LP_DEAD items. We > > > can simply increment the vacrel->vm_new_visible_pages and check > > > whether the page is frozen. > > > > My idea with the assert was sort of to codify the expectation that the > > page couldn't have been all-visible because of the dead items. But > > perhaps that is obvious. But you are right that the if statement is > > not needed. Perhaps I ought to remove the asserts as they may be more > > confusing than helpful. > > Thinking about this more, I think it is better without the asserts. > I've done this in attached v3.
I liked this version more. I agree that the asserts were causing some confusion. nitpick: + /* Count the newly all-frozen pages for logging. */ AFAIK, we do not use periods in the one line comments. Other than that, the patch looks good to me. -- Regards, Nazir Bilal Yavuz Microsoft
