[LLVMbugs] [Bug 2162] New: Migrate away from Value::getName()

[bugzilla-daemon at cs.uiuc.edu]

http://llvm.org/bugs/show_bug.cgi?id=2162

           Summary: Migrate away from Value::getName()
           Product: libraries
           Version: 1.0
          Platform: PC
        OS/Version: All
            Status: NEW
          Keywords: slow-compile
          Severity: normal
          Priority: P2
         Component: Core LLVM classes
        AssignedTo: unassignedbugs at nondot.org
        ReportedBy: sabre at nondot.org
                CC: llvmbugs at cs.uiuc.edu


Internally value names are stored in a StringMap, which is an efficient data
structure for finding and uniquing and autorenaming names that are in the
symbol table.  This is goodness and it's FAR more efficient than something like
std::map<std::string, Value*> which is what we used to use.

The disadvantage of this is that the map doesn't hold an std::string, which
means that (in particular) Value::getName() is extremely expensive.  It is
expensive because it *copies* the name of the value into an std::string to
return it.  This means that all clients of it that matter should move off of it
to reduce our compile times.  This will significantly reduce heap thrashing.

There are many ways to improve the situation.  For example:

1) This like that asmwriter that need to print out the string can move to using
Value::getNameStart()+Value::getNameLen(), which are both very efficient.

2) The nasty std:string Tmp = V1->getName(); V1->setName(""); V2->setName(Tmp)
idiom can be replaced with the *far* more efficient and nicer V2->takeName(V1)
call.

3) I common pattern is copy the name of some value with a suffix.  We should
add a helper to value for this, something like V1->copySuffixedName(V2, "i") or
something, which adds ".i".

4) It is also very common to use a numeric suffix, with this pattern:
V2->setName(V1->getName()+"."+utostr(i))   This is expensive for many reasons:
utostr allocates a string, the concatenations do as well, etc.  This is
generally horrible for memory use and performance.  It would be better to add a
V1->copySuffixedName(V2, i) to handle this, and implement it efficiently.

5) setName(std::string) is also fairly expensive if the input is not a string. 
Clients should start using the setName version that takes a const char* +
length for full generality, or just an const char* if they don't care about
nul's in the string.

6) Any uses of getName() that occur in debug code or in other places that we
don't care about should migrate over to using Value::getNameStr().  This will
allow us to identify places that we haven't look at yet.

When the full audit + migration is done, we can remove Value::getName()
entirely.

-Chris


-- 
Configure bugmail: http://llvm.org/bugs/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.