Why are empty Indexed*Identifier checked for ref counting?

[Milian Wolff]

David Nolden, 05.05.2010:
> 2010/5/5 Milian Wolff <mail at milianw.de>:
> > On Wednesday, 5. May 2010 20:00:42 David Nolden wrote:
> >> 2010/5/5 Milian Wolff <mail at milianw.de>:
> >> > Hey all, esp. David:
> >> > 
> >> > Why are IndexedQualifiedIdentifier and IndexedIdentifier checking
> >> > shouldDoReferenceCounting when they are empty? I.e. their ctors
> >> > without any arguments.
> >> 
> >> Because they still have an item assigned, the "empty" one, so also
> >> reference-counting has to be done for it. This is the "correct" way to
> >> do it, and is consistent with the way an indexed identifier is
> >> converted into a real identifier: Based on the index, it retrieves the
> >> item from the item-repository (see Identifier::Identifier(uint
> >> index)).
> >> 
> >> I've also thought a few times to treat empty identifiers by giving
> >> them a "zero" index instead of the
> >> emptyConstantIdentifierPrivateIndex() thing, but that would need a lot
> >> of special handling, also in the function mentioned above.
> > 
> > What would be more special compared to IndexedString that already uses
> > index == 0 for empty indizes?
> 
> The difference is that for the empty string, we don't need the "item"
> from the repository, while for the empty identifier we need.

One more stupid question from my side: Why do we need the item? It's empty 
after all. Or is this because we need to save that it is empty? IndexedStrings 
are also saved somehow, how is it done there?

> >> > And shouldn't/couldn't we use m_index(0) for the empty case, just like
> >> > with IndexedStrings? That would also speed up KDevelop, since right
> >> > now e.g. the IndexedQualifiedIdentifier in the CodeModel always goes
> >> > through this no matter what. The question is of course whether it
> >> > should do duchainrefcounting at that point or not ;-)
> >> 
> >> See above, the problem is that at some point we need to convert the
> >> index to the item through the item repository.
> >> 
> >> Regarding the next message:
> >> Yes of course, reference-counting is only done for stuff that is being
> >> saved to the disk, that's the interesting part about it this reference
> >> counting, and also the reason why the reference-counting for the empty
> >> identifiers you mentioned should be pretty irrelevant, because mostly
> >> shouldDoDUChainReferenceCounting should do nothing more than
> >> "if(false) return;".
> > 
> > It's still in the top then of most expensive things in the call graphs
> > though...
> 
> Not in any of the profilings that I did..

See [1]. It's a debugbuild, but relwithdebinfo should show similar costs just 
not the ctors as the culprits since they are inlined. Esp. if you compare the 
total fetch instr cost in debug and relwithdebinfo the difference is only a few 
percent afair.

Esp. interesting: group by elf object and take a look at 
kdevplatformlanguage.so the top 10.

Anyways you are right in one regard which I must have misinterpreted before: 
the empty ctors are not in that list so I probably only blew a lot of hot air 
:) Sorry for that.

[1]: http://milianw.de/files/prof.bz2

> > Also: such things would really help in the doc comments. You might find
> > them trivial but anyone else outside your brain needs to come up with
> > your reasoning ;-)
> > 
> > So if possible, please add some documentation.
> 
> Well, I even blogged about this. It's a basic concept of the
> persistent duchain, and it is used throughout all the duchain storage
> mechanisms. I'm also quite sure that there is some place where it's
> explained in a comment.

a blog is of course not enough... I'll hunt a bit deeper in the API for more 
information.

thanks
-- 
Milian Wolff
mail at milianw.de
http://milianw.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20100505/5d105a33/attachment.sig>