[PATCH] D14910: [RuntimeDyld] DenseMap -> std::unordered_map

[Duncan P. N. Exon Smith via llvm-commits]

SGTM.

> On 2015-Dec-01, at 11:28, Lang Hames <lhames at gmail.com> wrote:
> 
> I've LGTM'd the patch because I think Keno's right that unordered_map is a better structure to use here.
> 
> If DenseMap isn't clearing memory on clear(), that seems worthy of a separate bug report.
> 
> Cheers,
> Lang.
> 
> On Mon, Nov 30, 2015 at 9:00 PM, Duncan P. N. Exon Smith via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> Would using a std::unique_ptr<RelocationList> in a DenseMap solve the
> same problem?
> 
> If DenseMap doesn't release memory on clear(), maybe there's API we
> should add for that?
> 
> > On 2015-Nov-24, at 12:45, Keno Fischer via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> >
> > Yes, I'm agreeing with you, which is why I switched it to unordered_map instead of map as in the original patch. The primary concern here is memory usage, rather than performance. DenseMap is an absolutely aweful data structure to use here (perhaps it also has a bug that has it fail to reclaim memory upon being cleared - I'm not sure). When running the julia test suite without this patch, this DenseMap consumes 2.5GB (with this patch the map is at most a handful of MB at peak). Either std::map or std::unordered_map do fine here.
> >
> > On Tue, Nov 24, 2015 at 2:06 PM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> > On Tue, Nov 24, 2015 at 10:58 AM, Keno Fischer via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> > loladiro retitled this revision from "[RuntimeDyld] DenseMap -> std::map" to "[RuntimeDyld] DenseMap -> std::unordered_map".
> > loladiro updated this revision to Diff 41064.
> > loladiro added a comment.
> >
> > Tested with unordered_map. Seems marginally better in my tests, but probably not statistically significant given my methodology. Still there's no reason the map should be ordered, so might as well use an unordered_map.
> >
> > But DenseMap isn't ordered either, so this doesn't remove ordering - it was never there.
> >
> > Might be worth having some more numbers before we'd make that change...
> >
> >
> >
> > Repository:
> >   rL LLVM
> >
> > http://reviews.llvm.org/D14910
> >
> > Files:
> >   lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
> >   lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h
> >
> > Index: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h
> > ===================================================================
> > --- lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h
> > +++ lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h
> > @@ -30,6 +30,7 @@
> >  #include "llvm/Support/SwapByteOrder.h"
> >  #include "llvm/Support/raw_ostream.h"
> >  #include <map>
> > +#include <unordered_map>
> >  #include <system_error>
> >
> >  using namespace llvm;
> > @@ -229,7 +230,7 @@
> >    // Relocations to sections already loaded. Indexed by SectionID which is the
> >    // source of the address. The target where the address will be written is
> >    // SectionID/Offset in the relocation itself.
> > -  DenseMap<unsigned, RelocationList> Relocations;
> > +  std::unordered_map<unsigned, RelocationList> Relocations;
> >
> >    // Relocations to external symbols that are not yet resolved.  Symbols are
> >    // external when they aren't found in the global symbol table of all loaded
> > Index: lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
> > ===================================================================
> > --- lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
> > +++ lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
> > @@ -98,11 +98,11 @@
> >      // The Section here (Sections[i]) refers to the section in which the
> >      // symbol for the relocation is located.  The SectionID in the relocation
> >      // entry provides the section to which the relocation will be applied.
> > -    int Idx = it->getFirst();
> > +    int Idx = it->first;
> >      uint64_t Addr = Sections[Idx].LoadAddress;
> >      DEBUG(dbgs() << "Resolving relocations Section #" << Idx << "\t"
> >                   << format("%p", (uintptr_t)Addr) << "\n");
> > -    resolveRelocationList(it->getSecond(), Addr);
> > +    resolveRelocationList(it->second, Addr);
> >    }
> >    Relocations.clear();
> >
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>