Skip to content

Conversation

@derrickstolee
Copy link

The unpack_trees() method has many uses, and each can have
different performance characteristics. Create trace2 regions
around each call so we can measure the cost of this method
in each situation.

These instances were found by grepping for the
setup_unpack_trees_porcelain() method, which is called before
every actual call to unpack_trees(). Searching for
unpack_trees() finds many false-positives in comments
and documentation.

This is for gathering data on the impact unpack_trees()
is having on different commands. This PR should wait for
the 2.21.0 update (#122), but I'll use it to generate an
installer for local testing in the meantime.

The unpack_trees() method has many uses, and each can have
different performance characteristics. Create trace2 regions
around each call so we can measure the cost of this method
in each situation.

These instances were found by grepping for the
setup_unpack_trees_porcelain() method, which is called before
every actual call to unpack_trees(). Searching for
unpack_trees() finds many false-positives in comments
and documentation.

Signed-off-by: Derrick Stolee <[email protected]>
@jeffhostetler
Copy link

Let's put this on top of #133 that I just merged into
vfs 2.21. It has some other exploratory trace regions that this will go well with.

I know this is a bit of a nit, but can you
phrase the regions as trace2_region_enter("exp", "checkout/unpack-trees", the_repo)
then they will all be under the "exp" category. (I know this didn't exist before yesterday
and seems arbitrary.)

I have a region depth limit in the event target (so we don't accidentally dump rows for
a full recursive tree walk to the DB). This depth is set at 2. I think your messages and
the ones I added yesterday will nest and be at the limit, so we're OK. But later if we want
to do more exploratory (temporary) tracing, I can let the "exp" category go a little deeper.
The thought is that we'd never push "exp" category upstream, and that "real" (permanent)
tracing would always use a well-defined category, like "index" or "status".

Also, there is a call to unpack_trees() in builtin/checkout.c : reset_tree() (nicely hidden in
the switch statement).

Copy link

@jeffhostetler jeffhostetler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some earlier notes on the main page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants