On Mon, Jun 30, 2014 at 6:33 PM, Julian Foad <julianf...@btopenworld.com>
wrote:
> Hi Stefan.
>
> fs_fs.c:
> > "### When a specific piece of information needs to be read from disk,
> a" NL
> > "### data block is being read at once and its contents are being
> cached." NL
> > "### ...
> > "### block-size is 64 kBytes by
> default." NL
> > "# " CONFIG_OPTION_BLOCK_SIZE " =
> 64" NL
>
> ^^ Can you state the size units explicitly in the commentary please? (If
> an admin has changed the value to say "4" or "4096" then the implicit clue
> of "64" matching "64 kBytes" would be lost.)
>
> >
> "###"
> NL
> > "### The log-to-phys index maps data item numbers to offsets within
> the" NL
> > "### rev or pack file. ...
> > "### l2p-page-size is 8192 entries by
> default." NL
> > "# " CONFIG_OPTION_L2P_PAGE_SIZE " =
> 8192" NL
> >
> "###"
> NL
> > "### The phys-to-log index maps positions within the rev or pack file
> to" NL
> > "### to data items, ...
> > "### For source code repositories, this should be about 16x the
> block-size." NL
> > "### Must be a power of
> 2." NL
> > "### p2l-page-size is 1024 kBytes by
> default." NL
> > "# " CONFIG_OPTION_P2L_PAGE_SIZE " =
> 1024" NL
>
> ^^ Same for this one.
>
> fs.h:
> > typedef struct fs_fs_data_t
> > { ...
> > /* Rev / pack file read granularity. */
> > apr_int64_t block_size;
>
> And here? (Here it's bytes not kilobytes, I believe.)
>
> > /* Capacity in entries of log-to-phys index pages */
> > apr_int64_t l2p_page_size;
>
> For disambiguation, maybe call it "l2p_page_entries" or
> "l2p_page_size_entries" instead? Otherwise my instinct is to assume the
> same units as p2l_page_size.
>
> > /* Rev / pack file granularity covered by phys-to-log index pages */
> > apr_int64_t p2l_page_size;
>
> And here?
>
> > ... }
>
> The initialization code converts kilobytes to bytes:
>
> fs_fs.c:
> > if (ffd->format >= SVN_FS_FS__MIN_LOG_ADDRESSING_FORMAT)
> > {
> > SVN_ERR(svn_config_get_int64(config, &ffd->block_size,
> > CONFIG_SECTION_IO,
> > CONFIG_OPTION_BLOCK_SIZE,
> > 64));
> > SVN_ERR(svn_config_get_int64(config, &ffd->l2p_page_size,
> > CONFIG_SECTION_IO,
> > CONFIG_OPTION_L2P_PAGE_SIZE,
> > 0x2000));
> > SVN_ERR(svn_config_get_int64(config, &ffd->p2l_page_size,
> > CONFIG_SECTION_IO,
> > CONFIG_OPTION_P2L_PAGE_SIZE,
> > 0x400));
> >
> > ffd->block_size *= 0x400;
> > ffd->p2l_page_size *= 0x400;
>
> But no such multiplier for l2p_page_size? Oh, that's because it's measured
> in entries rather than bytes/kilobytes. That confused me when I first read
> this: it looked like an accidental omission.
>
> > }
> > else
> > {
> > /* should be irrelevant but we initialize them anyway */
> > ffd->block_size = 0x1000;
> > ffd->l2p_page_size = 0x2000;
> > ffd->p2l_page_size = 0x100000;
>
> At first I thought these numbers were different from above, then I noticed
> the multipliers above and decided they're the same end result, but then
> hours later I noticed the 'block size' here is _not_ equal to 64 x 0x400
> (which would be 0x10000 not 0x1000).
>
> I know this last set of values "should be irrelevant" but we should avoid
> hard-to-spot differences like this one. Can you write this so there are not
> so many different numbers in the source code? How about using defined
> constants, since each one appears approx. 4 times including the config file
> commentary?
>
Most of this is covered by r1616613. I did not, however, rename
l2p_page_size to keep the actual code churn low.
-- Stefan^2.
