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

[Lang Hames via llvm-commits]

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151201/2f74d784/attachment.html>