Samuel Thibault <samuel.thiba...@gnu.org> writes:
> Maksym Planeta, le Sat 07 Apr 2012 21:42:04 +0300, a écrit :
>> Samuel Thibault <samuel.thiba...@gnu.org> writes:
>>
>> > Maksym Planeta, le Sat 07 Apr 2012 19:51:56 +0300, a écrit :
>> >> Here is initialization code from pager_alloc():
>> >> if (INDIRECT_PAGEMAP(size)) {
>> >> alloc_size = INDIRECT_PAGEMAP_SIZE(size);
>> >> init_value = (dp_map_t)0;
>> >>
>> >> And from pager_extend():
>> >> for (; i < INDIRECT_PAGEMAP_ENTRIES(new_size); i++)
>> >> new_mapptr[i].indirect = (dp_map_t)0;
>> >>
>> >> As you can see instead of NULL, (dp_map_t)0 is used.
>> >
>> > And can be put into pager->map in pager_truncate, ok. I'm however not
>> > sure we really want to put ifs everywhere. The comment in the truncation
>> > says
>> >
>> > /* We are truncating to a size small enough that it goes to using
>> > a one-level map. We already have that map, as the first and only
>> > nonempty element in our indirect map. */
>> >
>> > i.e. the code assumes that map[0].indirect is not NULL. I'd say we
>> > should rather allocate an empty map in such case, to keep the rest of
>> > the code simple.
>> >
>>
>> And what is the alternative for ifs? longjumps and setjumps?
>
> No: as I said, allocate an empty map, so that the existing code can poke
> at it without testing for its presence or not.
>
>> Purpose of this conditions is checking whether map (or submap) is
>> already empty.
>
> Not empty, but allocated.
>
So, if, for instance, only one page of large object (that needs indirect
mapping) was evicted, the whole map would be allocated? And what the
purpose of two-level system in this case?
>> >> > There is also an issue with
>> >> >
>> >> > + if (!pager->map) {
>> >> > + invalidate_block (pager_offset);
>> >> > + goto done;
>> >> > + }
>> >> > pager_offset = pager->map[f_page];
>> >> >
>> >> > at that point, pager_offset is not initialized yet...
>> >> >
>> >>
>> >> invalidate_block is a macro that sets pager_offset, so, really,
>> >> pager_offset shouldn't been initialized yet.
>> >
>> > It sets the *content* pointed by pager_offset. It does not set the
>> > pager_offset pointer.
>>
>> pager_offset is not a pointer, it is a union.
>
> Oops, indeed, sorry about that. I'm still wondering, however: rather
> than a goto, why not just putting pager_offset = pager->map[f_page] in
> the else part?
>
pager_offset = pager->map[f_page] could be just put in else block, but I
did so because "goto done" shows clearer that everything was done and
execution could be move to finalization part. Just a matter of style. Do
you think that this should be changed?
Regards,
Maksym Planeta.
