RFR (S) 8211926: Catastrophic size_t underflow in BitMap::*_large methods

[Thomas Stüfe]

Hi Kim,

On Sat, Nov 10, 2018 at 6:55 PM Kim Barrett <kim.barrett at oracle.com> wrote:
>
> > On Nov 10, 2018, at 7:58 AM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
> > - I find it confusing that we use the same type (idx_t) both for word-
> > and bit-indices. It would make the code a bit more readable if those
> > were separate types (even if they both were to alias to size_t,
> > ultimately).
>
> I dislike this too.  I’d be interested in seeing a proposal to fix that, though
> I worry about the fanout.  I *think* clients mostly operate in bits and the
> use of words is mostly limited to the implementation, but I haven’t done
> the searches needed to check that.

Should I have the time I'll give this a try. Not sure when I will find
the time though.

I plan, at some point, to exchange my own hand-grown OccupancyBitmap
in metaspace coding with this BitMap class. Maybe that will be a good
time to look at it more closely.

>
> > - Generally, I find putting the decision between "large" and
> > "non-large" APIs off to the caller a bit strange. As a user of bitmap,
> > I cannot know at which point which variant would be better. I would
> > prefer the class itself handling that.
>
> I was thinking much the same thing while reviewing this change.
> Maybe some of the public API in this area should be revisited.
> In particular, I was surprised that one might call “large” functions
> directly, rather than calling hinted functions with a “large” hint.
> Perhaps the “large” functions should be private helpers that just
> do what they are told (maybe with an assert as before), and the
> calls to them should be based on the hint and the runtime size
> check they are now performing.  Or maybe the whole hint thing
> should be reconsidered.  I’d be interested in a proposal in this
> area too.

I would consider switching between loops and memset to be an internal
tuning parameter which the caller should not have to bother with. I
think hinting makes sense when I know something the code cannot know.
But there are no hidden information here beside the range size. Unless
I misunderstand the problem.

Thanks, Thomas
>