On Thu, 7 Apr 2022 13:31:31 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
> Since the JVMTI heap walk usually walks the whole heap, the bitmap may not be
> as sparse as we think. It would materialize a lot - possibly all - of its
> fragments. The increased memory consumption during JVMTI heap walk may mess
> with JVMTI agents using heapwalking for analysis in OOM situations.
The on-demand allocation of fragments should be a lot better than what I
proposed before (in the Lilliput PR). The Java heap is typically never fully
occupied, Java OOM situations aside. Heap walking may also start from a single
object, in which case we would likely only walk a small subset of the heap. The
current solution already allocates 2x4000x8 bytes by default for stashing
locked and hashed headers. Also, only because Java runs out of memory on the
heap doesn't mean that it's likely that we're also running out of native heap.
The maximum memory usage should be around 1/64th of the Java heap size on
default object alignment.
> About this PR:
>
> You moved Bitset to general-purpose land. Now its a general purpose class,
> and should be more versatile and better documented. A comment would be good,
> at least for the structure itself. Proposal (feel free to modify/extend):
>
> ```
> BitSet is a sparse bitmap describing a memory range. It holds one bit per
> xxx-aligned
> address. Its underlying backing memory is allocated on-demand only, in
> yyy-sized
> fragments. Fragments are never deleted. The underlying memory is allocated
> from C-Heap.
> ```
Yes, I will do that.
> I would also rename the function, since it is very unclear what the
> difference is between BitMap and BitSet. E.g. "AddressMap"
> "SparseAddressMap", or similar.
As you noted elsewhere, it is currently very heap/object-centric. We could name
it ObjectBitSet or something like that for now. I would not want to implement
the (reasonable) abstractions that you're proposing below, simply because it's
not used as such, currently. Whenever we need such use, or have a desire to
merge BitMap and ObjectBitSet, then we shall consider the abstractions, but now
it seems a bit premature.
> fragment size xxx and alignment yyy would ideally be configurable at compile
> time. At the moment, it feels very "heapish". It uses oop. It uses
> LogMinObjAlignmentInBytes. Ideally we would not include oop.hpp, and not know
> about oops.
> Especially the dependency on LogMinObjAlignmentInBytes is not optimal: that
> is a runtime variable depending on `-XX:ObjectAlignmentInBytes`. It would be
> good if that could be a compile time constant. Right now, IIUC,
> `BitSet<F>::addr_to_bit()` will always load the alignment from memory.
Yes, but we *do* want to use it as object bitmap, and thus should know about
ObjAlignmentInBytes, and this is determined at run-time only.
> As a general purpose structure, some gtests would be nice. Maybe in some
> future RFE.
Indeed. I will look into adding some.
> A general thought, maybe for a future RFE:
>
> We now have BitMap and BitSet, which do almost the same thing. Instead of
> having a new class BitSet, whose job is to be sparse, we could give BitMap
> the ability to be sparse. We'd save code, reuse the rich BitMap interface and
> get all the test code for free.
>
> One way to do that would be to make BitMap responsible for committing its
> underlying backing memory, in chunks, on-demand only. To do that BitMap would
> need a secondary map - a commit map - to remember which fragments of its
> backing memory are committed. Commit map would have a bit per 64M fragment.
>
> As a sketch:
>
> ```
> class BitMap {
>
> // bitmap holding one bit per 64M fragment of the underlying bitmap
> backing memory
> BitMap _commit_map;
>
> inline void set_bit(idx_t bit) {
> // - find out which fragment the bit lives in
> // - if (commitmap[fragmentid] == 0) { commit_fragment();
> commitmap[fragmentid]=1; }
> // - set bit
> }
>
> // same for clear_bit()
>
> bool at(idx_t index) const {
> // - find out which fragment the bit lives in
> // - if (commitmap[fragmentid] == 0) { return false; } // bit has never
> been set, is therefore zero
> // - else return the actual bit
> }
>
> }
> ```
>
> The sparse-handling part would have to be switchable at compile time so that
> we won't pay for it in non-sparse BitMap.
>
> I believe this solution could be simpler than a new separate utility class,
> and be more versatile. Would be interesting to know @kimbarrett 's opinion.
>
> But this can be done of course in a separate RFE. If at all.
Yes, let's do that as follow-up RFE.
Thanks,
Roman
-------------
PR: https://git.openjdk.java.net/jdk/pull/7964
