On Thu, 28 Jan 2021 12:07:52 GMT, Thomas Schatzl <[email protected]> wrote:
>> I assume that tier1-3 includes the SA tests :) >> >> Looks good other than that nit. > > Hi David, > >> _Mailing list message from [David Holmes](mailto:[email protected]) on >> [serviceability-dev](mailto:[email protected]):_ >> >> On 28/01/2021 7:09 pm, Thomas Schatzl wrote: >> >> > On Thu, 28 Jan 2021 05:13:57 GMT, David Holmes <dholmes at openjdk.org> >> > wrote: >> > > > Please review this change which merges ImmutableSpace into >> > > > MutableSpace, >> > > > eliminating the former. There were no interesting uses of >> > > > ImmutableSpace, >> > > > other than as the base class for MutableSpace. The name >> > > > ImmutableSpace is >> > > > kind of a misnomer given that usage. >> > > > Testing: >> > > > mach5 tier1-3 >> > > >> > > src/hotspot/share/gc/parallel/mutableSpace.hpp line 47: >> > > > 45: // >> > > > 46: // Invariant: bottom() <= top() <= end() >> > > > 47: // top() is inclusive and end() is exclusive. >> > > >> > > If end() is exclusive then shouldn't the invariant be `< end()`? >> > >> > I also think that top() is also exclusive as in other collectors. >> > @dholmes-ora : e.g. bottom == top == end means the space is empty. These >> > two lines are not disagreeing with each other. >> >> If one is exclusive and one is inclusive then I don't see how they can >> be equal, as that implies they are then both inclusive and exclusive at >> the same time. ?? If end() is exclusive then I would expect an empty >> space to be one where bottom and end are adjacent, not coincident. >> >> Cheers, >> David > > The original comment about top() being inclusive is wrong. top() is also > exclusive like in all other collectors as stated elsewhere in my review > comment. My "also" in "I also think that top() is also exclusive as in other > collectors." probably threw you off after re-reading it, which is wrong. > Sorry. > > Maybe some examples help: > > bottom = 200, top = 200, end = 200 is an "empty" space (i.e. is of size > zero). Whether that empty space is "free" or "fully allocated" or both or > neither is another question :) > > bottom = 200, top = 200, end = 201 contains one word and is (completely) free > (not allocated into at all). > > bottom = 200, top = 201, end = 201 contains one word and is full(y allocated). > > Top/end are exclusive, and bottom inclusive as does the code assume from what > I can tell by quickly looking at it. Still the invariant is bottom <= top <= > end in all cases. > > Thanks, > Thomas What @tschatzl said. The comment saying `top()` is inclusive is simply wrong. I'll fix it before integrating. ------------- PR: https://git.openjdk.java.net/jdk/pull/2271
