On Fri, Nov 13, 2015 at 1:44 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Nov 13, 2015 at 4:48 AM, Masahiko Sawada <sawada.mshk@gmail.com>
> wrote:
>>
>>
>> Thank you for reviewing the patch.
>>
>> I changed the patch so that the visibility map become the page info
>> map, in source code and documentation.
>>
>
> One thing to notice is that this almost doubles the patch size which
> might makes it slightly difficult to review, but on the other hand if
> no-body opposes for such a change, this seems to be the right direction.
I believe that it's going to right direction.
But I think we didn't get consensus about this changes yet, so it might go back.
>
>> And fixed review comments I received.
>> Attached v22 patch.
>>
>> > I think both the above cases could happen for frozen state
>> > as well, unless you think otherwise, we need similar handling
>> > for frozen bit.
>>
>> It's not happen the situation where is all-frozen and not all-visible,
>> and the bits of visibility map are cleared at the same time, page
>> flags are as well.
>> So I think it's enough to handle only all-visible situation. Am I
>>
>> missing something?
>>
>
> No, I think you are right as information for both is cleared together
> and all-visible is superset of all-frozen (means if all-frozen is set,
> then all-visible must be set), so it is sufficient to check visibility
> info in above situation, but I feel we can update the comment to
> indicate the same and add an Assert to ensure if all-frozen is set
> all-visibile must be set.
I agree.
I added Assert() macro into lazy_scan_heap() and some comments.
Attached v23 patch.
Regards,
--
Masahiko Sawada
