Hi Olav,
Many thanks for the review.
> * Performance issue in SectorBuffer.cpp:
>
> -you have added the call to readPages() to
> SectorBuffer::readSector(). If I understand the code correctly, this
> results in that the checksum is computed for all pages in the read
> sector. With the default sector size and page size this results in 16
> pages being checksummed. For sequential read this is OK but for random
> page accesses you will checksum 15 pages that are never used. One of
> the
> ideas with the SectorCache was to read the entire sector instead of
> just
> a page for each io operation under the assumption that this was "almost
> free". Having the checksum computation here makes this "almost free" a
> little "less free". My suggestion is to move the check sum verification
> to the SectorBuffer::readPage. Then we would checksum only pages that
> we
> actually use.
That is a good point. I tried to do that and failed on the first run (I got
an assertion as page was read from SectorBuffer, then modified, then written
to SectorBuffer and then read again - the second time checksum was
outdated).
It is a little bit hairy with checksums in the SectorCache/SectorBuffer.
Moving the verification into SectorBuffer::readPage requires checksum update
in SectorBuffer::writePage .Checksum update in SectorBuffer::writePage
,without modifying the rest, would cause checksums to be updated twice for
the same page (IO::writePages and SectorBuffer::WritePage) - that is not
what we want. Simply removing the checksum calculation from IO::writePages
is not going to
work either, there are currently circumstances where IO::writePage is called
without SectorCache modifications (e.g shutdown)
So the only option is to look at each place, where either
SectorCache::writePage or IO::writePage(s) or both are called and update
checksums on a page before that. I'm afraid this could be harder to maintain
(i.e each time one changes the code that has writePages in it, programmer
would need to figure out whether checksum calculation is already done or not
and insert updateChecksum code on the right place).
So I'd tend to keep the current code as is for now, with checksum
calculation as close to IO as possible. If benchmarks find the performance
horrible, then a closer look is needed.
But thank you for performance warning, I would not have figured out that.
Now to other points
> -IO::pageChecksum(): I would have called this IO::computeChecksum
> (reason: the first time I saw a call to this in the code I assumed this
> read out the check sum field from the page)
Agree.
> -IO::writePage(): Great simplification and clean-up by replacing
> all this with a single call to writePages(). One very minor difference
> introduced by this change: you now update "++flushWrites" instead of
> "++writes". Do you know if this has any consequences on anything?
Good catch. Search does not reveal that these numbers are used anywhere.
They may have served Jim's debugging purposes ;)
> -IO::readHeader(): very minor comment: I did not immediately
> understand the constant "bufSize = 32*1024". Do we have a constant for
> MAX_PAGE_SIZE? Or possibly call the local constant maxPageSize=
> 32*1024.
> Would at least for me make the code easier to understand the first time
> I read it.
Agree. There is no constant for it so far, I'll change bufSize to
maxPageSize however.
Thank you again,
Vlad
Off-topic: An example SectorCache with only 6% useful data (1 page out of
16) does not sound "almost free"y. There is an overhead of copying data
around from filesystem cache to SectorCache plus overhead of copying data
from SectorCache to PageCache. Maybe SectorCache was in fact not *that* good
idea. I actually forgot the whole discussion about why it was needed, the
only thing I recall is your idea to force prefetch in filesystem cache by
reading more than single page each time, but ignore the extra data that was
read. That idea was not too bad and, and could be implemented with just
couple of lines in IO::readPages.
> -----Original Message-----
> From: Olav.Sandstaa@stripped [mailto:Olav.Sandstaa@stripped]
> Sent: Wednesday, July 02, 2008 11:03 PM
> To: Vladislav Vaintroub
> Cc: commits@stripped
> Subject: Re: bzr commit into mysql-6.0-falcon branch (vvaintroub:2717)
> WL#4173
>
