Before, I thought that the proportion of `lookup_object()` is not very
large(11.47%), so
I didn't care about it. But Christian strongly recommends that I use
`trace_printf()` to
observe the number of calls to `lookup_object()`.
Here is an amazing fact:
The number of calls to `lookup_object()` before and after using my
patch are 0 and
522709 respectively. Therefore, I am very surprised, why do we have
these additional calls?
(gdb)bt#0 lookup_object (r=r@entry=0x5555558b8cc0 <the_repo>,oid=oid@entry=0x5555558b8980<oi>)atobject.c:92#1 0x0000555555665572 in lookup_commit (r=0x5555558b8cc0 <the_repo>,oid=0x5555558b8980<oi>)atcommit.c:62#2 0x00005555556edff5 in parse_object_buffer (r=0x5555558b8cc0<the_repo>,oid=oid@entry=0x5555558b8980<oi>,type=OBJ_COMMIT,size=788,buffer=0x5555558d0080,eaten_p=eaten_p@entry=0x7fffffffcc0c)atobject.c:214#3 0x000055555571da42 in get_object (ref=ref@entry=0x7fffffffcf30,deref=deref@entry=0,obj=obj@entry=0x7fffffffcc90,oi=oi@entry=0x5555558b8980<oi>,err=err@entry=0x7fffffffcf10)atref-filter.c:1774#4 0x000055555571fdc2 in populate_value(ref=ref@entry=0x7fffffffcf30,err=err@entry=0x7fffffffcf10)atref-filter.c:1999#5 0x00005555557202eb in get_ref_atom_value(ref=ref@entry=0x7fffffffcf30,atom=0,v=v@entry=0x7fffffffcea8,err=err@entry=0x7fffffffcf10)atref-filter.c:2033#6 0x00005555557212d6 in format_ref_array_item(info=info@entry=0x7fffffffcf30,format=format@entry=0x7fffffffd0f0,final_buf=final_buf@entry=0x7fffffffd060,error_buf=error_buf@entry=0x7fffffffcf10)atref-filter.c:2627#7 0x00005555555859d8 in batch_object_write (scratch=0x7fffffffd060,opt=0x7fffffffd0d0,data=<optimizedout>,obj_name=0x0)atbuiltin/cat-file.c:224
After printing the call stack of `lookup_object()`, we can know that
the `parse_buffer()`
is calling them. A very straightforward idea, can we avoid calling
this function?
In `parse_object_buffer()`, `parse_blob_buffer()`, ``parse_tree_buffer()`,
`parse_commit_buffer()`, and `parse_tag_buffer()` parse the object
data, and then store
it in `struct object *obj`, finally return it to the caller.
`get_object()` will feed the `obj` to `grab_values()`, and then
`grab_values()` will feed the
`obj` to `grab_tag_values()`, `grab_commit_values()`, which can fill
the object info in `obj` to
implement some atom, e.g. `%(tag)`, `%(type)`, `%(object)`, `%(tree)`,
`%(numparent)`,`%(parent)`.
It is worth noting that `%(objectname)`, `%(objecttype)`,
`%(objectsize)`, `%(deltabase)`, `%(rest)`,
`%(raw)` are did not appear in them, this means that we can avoid
parsing object buffer when we
don't use those atoms which require `obj`'s information!
After some processing and adaptation, I made the patch which can skip
`parse_object_buffer()`
in some cases, this is the result of the performance test of
`t/perf/p1006-cat-file.sh`:
We can see that the performance of `git cat-file --batch` has been a
certain improvement!
Tell a joke: removing 1984531500 if checking can reduce the startup
time of GTA5 by 70%. :-D
Currently the patch has not been submitted to the mailing list, let us
wait a bit...
--
ZheNing Hu
Here is an amazing fact:
The number of calls to `lookup_object()` before and after using my
patch are 0 and
522709 respectively. Therefore, I am very surprised, why do we have
these additional calls?
After printing the call stack of `lookup_object()`, we can know that
the `parse_buffer()`
is calling them.
s/the `parse_buffer()`/`parse_buffer()`/
or
s/the `parse_buffer()`/the `parse_buffer()` function/
Also: s/them/it/
A very straightforward idea, can we avoid calling
this function?
In `parse_object_buffer()`, `parse_blob_buffer()`, ``parse_tree_buffer()`,
`parse_commit_buffer()`, and `parse_tag_buffer()` parse the object
s/parse/we parse/
data, and then store
it in `struct object *obj`, finally return it to the caller.
Maybe: s/finally/and finally/
`get_object()` will feed the `obj` to `grab_values()`, and then
`grab_values()` will feed the
`obj` to `grab_tag_values()`, `grab_commit_values()`, which can fill
the object info in `obj` to
implement some atom, e.g. `%(tag)`, `%(type)`, `%(object)`, `%(tree)`,
`%(numparent)`,`%(parent)`.
It is worth noting that `%(objectname)`, `%(objecttype)`,
`%(objectsize)`, `%(deltabase)`, `%(rest)`,
`%(raw)` are did not appear in them, this means that we can avoid
s/are did not/don't/
parsing object buffer when we
don't use those atoms which require `obj`'s information!
After some processing and adaptation, I made the patch which can skip
s/the patch/a patch/
`parse_object_buffer()`
in some cases, this is the result of the performance test of
`t/perf/p1006-cat-file.sh`:
We can see that the performance of `git cat-file --batch` has been a
certain improvement!
Yeah, sure -8.1% or -17.5% is really nice! But why +10.0% for
`cat-file --batch-check`?
I think it's not very important. Because our optimization is skipping
parse_object_buffer(), git cat-file --batch-check will not set oi->contentp
by default, parse_object_buffer() will not be executed, Therefore, we did
not optimize `git cat-file --batch-check` at all. 10% may be small enough
for git cat-file --batch-check. The noise of environment even will cover it...
quoted
Tell a joke: removing 1984531500 if checking can reduce the startup
time of GTA5 by 70%. :-D
We can see that the performance of `git cat-file --batch` has been a
certain improvement!
Yeah, sure -8.1% or -17.5% is really nice! But why +10.0% for
`cat-file --batch-check`?
I think it's not very important. Because our optimization is skipping
parse_object_buffer(), git cat-file --batch-check will not set oi->contentp
by default, parse_object_buffer() will not be executed, Therefore, we did
not optimize `git cat-file --batch-check` at all. 10% may be small enough
for git cat-file --batch-check. The noise of environment even will cover it...
By the way, its performance may still be worse than "upstream/master", but it
will be better than before optimization.
Test HEAD~ this tree
------------------------------------------------------------------------------------
1006.2: cat-file --batch-check 0.10(0.09+0.01)
0.09(0.08+0.01) -10.0%
1006.3: cat-file --batch-check with atoms 0.09(0.07+0.02)
0.08(0.05+0.03) -11.1%
1006.4: cat-file --batch 0.61(0.59+0.02)
0.53(0.51+0.02) -13.1%
1006.5: cat-file --batch with atoms 0.60(0.57+0.02)
0.52(0.49+0.03) -13.3%
Test upstream/master this
tree
------------------------------------------------------------------------------------
1006.2: cat-file --batch-check 0.08(0.07+0.01)
0.10(0.07+0.02) +25.0%
1006.3: cat-file --batch-check with atoms 0.06(0.05+0.01)
0.08(0.08+0.00) +33.3%
1006.4: cat-file --batch 0.49(0.46+0.03)
0.53(0.50+0.03) +8.2%
1006.5: cat-file --batch with atoms 0.48(0.45+0.03)
0.51(0.48+0.02) +6.3%
Test upstream/master HEAD~
------------------------------------------------------------------------------------
1006.2: cat-file --batch-check 0.08(0.07+0.01)
0.10(0.07+0.03) +25.0%
1006.3: cat-file --batch-check with atoms 0.06(0.05+0.00)
0.08(0.08+0.00) +33.3%
1006.4: cat-file --batch 0.48(0.46+0.02)
0.60(0.57+0.03) +25.0%
1006.5: cat-file --batch with atoms 0.47(0.44+0.03)
0.63(0.59+0.04) +34.0%
Thanks.
--
ZheNing Hu
We can see that the performance of `git cat-file --batch` has been a
certain improvement!
Yeah, sure -8.1% or -17.5% is really nice! But why +10.0% for
`cat-file --batch-check`?
I think it's not very important. Because our optimization is skipping
parse_object_buffer(), git cat-file --batch-check will not set oi->contentp
by default, parse_object_buffer() will not be executed.
Do you think that if git cat-file --batch-check would set
oi->contentp, there would be no performance regression for `cat-file
--batch-check`?
Could you test that?
quoted
Therefore, we did
not optimize `git cat-file --batch-check` at all. 10% may be small enough
for git cat-file --batch-check. The noise of environment even will cover it...
By the way, its performance may still be worse than "upstream/master", but it
will be better than before optimization.
Nice that there is some improvement, but it would be better if it was
similar to "upstream/master".
Test HEAD~ this tree
------------------------------------------------------------------------------------
1006.2: cat-file --batch-check 0.10(0.09+0.01)
0.09(0.08+0.01) -10.0%
1006.3: cat-file --batch-check with atoms 0.09(0.07+0.02)
0.08(0.05+0.03) -11.1%
1006.4: cat-file --batch 0.61(0.59+0.02)
0.53(0.51+0.02) -13.1%
1006.5: cat-file --batch with atoms 0.60(0.57+0.02)
0.52(0.49+0.03) -13.3%
Yeah, your patch seems to be an overall improvement when the
ref-filter code is used.
Test upstream/master this
tree
------------------------------------------------------------------------------------
1006.2: cat-file --batch-check 0.08(0.07+0.01)
0.10(0.07+0.02) +25.0%
1006.3: cat-file --batch-check with atoms 0.06(0.05+0.01)
0.08(0.08+0.00) +33.3%
1006.4: cat-file --batch 0.49(0.46+0.03)
0.53(0.50+0.03) +8.2%
1006.5: cat-file --batch with atoms 0.48(0.45+0.03)
0.51(0.48+0.02) +6.3%
This means that some further performance improvements are still needed
both for --batch and --batch-check though.
Have you tried to see, using gprof or something else, what is still
degrading the performance compared to when the ref-filter code isn't
used?
We can see that the performance of `git cat-file --batch` has been a
certain improvement!
Yeah, sure -8.1% or -17.5% is really nice! But why +10.0% for
`cat-file --batch-check`?
I think it's not very important. Because our optimization is skipping
parse_object_buffer(), git cat-file --batch-check will not set oi->contentp
by default, parse_object_buffer() will not be executed.
Do you think that if git cat-file --batch-check would set
oi->contentp, there would be no performance regression for `cat-file
--batch-check`?
Could you test that?
Oh, I mean that if git cat-file --batch-check with its default format
"%(objectname) %(objecttype)
%(objectsize)", it will not have any optimization; But if git cat-file
--batch set with "%(contents)" or
some other atoms, it will indeed be optimized. See 1006.4:
Test this tree
HEAD~
---------------------------------------------------------------------------------------------
1006.2: cat-file --batch-check 0.15(0.12+0.02)
0.15(0.13+0.01) +0.0%
1006.3: cat-file --batch-check with basic atoms 0.12(0.10+0.01)
0.12(0.10+0.02) +0.0%
1006.4: cat-file --batch-check with contents atoms 0.66(0.63+0.02)
0.75(0.72+0.02) +13.6%
1006.5: cat-file --batch 0.61(0.57+0.04)
0.70(0.65+0.05) +14.8%
1006.6: cat-file --batch with atoms 0.58(0.57+0.01)
0.67(0.63+0.03) +15.5%
quoted
quoted
Therefore, we did
not optimize `git cat-file --batch-check` at all. 10% may be small enough
for git cat-file --batch-check. The noise of environment even will cover it...
By the way, its performance may still be worse than "upstream/master", but it
will be better than before optimization.
Nice that there is some improvement, but it would be better if it was
similar to "upstream/master".
Agree.
quoted
Test HEAD~ this tree
------------------------------------------------------------------------------------
1006.2: cat-file --batch-check 0.10(0.09+0.01)
0.09(0.08+0.01) -10.0%
1006.3: cat-file --batch-check with atoms 0.09(0.07+0.02)
0.08(0.05+0.03) -11.1%
1006.4: cat-file --batch 0.61(0.59+0.02)
0.53(0.51+0.02) -13.1%
1006.5: cat-file --batch with atoms 0.60(0.57+0.02)
0.52(0.49+0.03) -13.3%
Yeah, your patch seems to be an overall improvement when the
ref-filter code is used.
quoted
Test upstream/master this
tree
------------------------------------------------------------------------------------
1006.2: cat-file --batch-check 0.08(0.07+0.01)
0.10(0.07+0.02) +25.0%
1006.3: cat-file --batch-check with atoms 0.06(0.05+0.01)
0.08(0.08+0.00) +33.3%
1006.4: cat-file --batch 0.49(0.46+0.03)
0.53(0.50+0.03) +8.2%
1006.5: cat-file --batch with atoms 0.48(0.45+0.03)
0.51(0.48+0.02) +6.3%
This means that some further performance improvements are still needed
both for --batch and --batch-check though.
Have you tried to see, using gprof or something else, what is still
degrading the performance compared to when the ref-filter code isn't
used?
Yeah, gprof show that Number of calls of strbuf_add(), xstrdup() has increased
after using the logic of ref-filter. But at the same time, I noticed
that grab_person()
seems to be an area worth optimizing. grab_person() uses its parameter
"const char *who"
for type comparison, But after we added `enum atom_type` to
ref-filter, We can use it
for some comparisons. And there are two for() loops in grab_person(),
and we can merge
them into one. With this patch [1], there is a slight improvement in
performance.
Test this tree
HEAD~
-------------------------------------------------------------------------------------------
1006.2: cat-file --batch-check 0.14(0.13+0.01)
0.15(0.14+0.01) +7.1%
1006.3: cat-file --batch-check with atoms 0.12(0.10+0.01)
0.12(0.09+0.02) +0.0%
1006.4: cat-file --batch-check with contents atom 0.66(0.65+0.01)
0.66(0.64+0.02) +0.0%
1006.5: cat-file --batch 0.60(0.57+0.02)
0.60(0.57+0.03) +0.0%
1006.6: cat-file --batch with atoms 0.58(0.53+0.04)
0.58(0.56+0.02) +0.0%
1006.7: cat-file --batch with person atoms 0.59(0.57+0.02)
0.60(0.56+0.04) +1.7%
It’s also worth mentioning that I found that grab_person() seems to be doing
repeated parsing which parse_object_buffer() may already be done.
parse_commit_buffer()
and parse_tag_buffer() have parsed part of the content of the object,
and used by
grab_tag_values() and grab_commit_values(). For the time being, I
think this is a kind of
shallow parsing, if we can let parse_object_buffer() do in-depth
parsing, it would be great.
We can save a lot of work in grab_person()... Of course this may be a
little difficult.
Thanks.
--
ZheNing Hu
[1]: https://github.com/adlternative/git/commit/cec0ee72e64d651c01d7a2a7fe17a4adab1ef0de