On Tue, 29 Sep 2020 09:22:18 GMT, Roman Kennke <rken...@openjdk.org> wrote:
>> I've also has problems with this assert in the past, and I think that the >> underlying assumption of the assert is wrong. >> I would not miss it. > > IMO it's ok to remove it. > However, it can be argued that is_in() should not check for 'in committed > memory' but 'in reserved space for heap', IOW > 'is this a pointer into our heap memory region?'. Or maybe this would be > CH::is_in_reserved() and we should change the > assert to that? Also, IMO we should not do anything that may trip barriers on > oop-iterators or similar GC-only paths. Almost always, what you want to do, is to assert the validity of heap pointers, where they are expected to be valid (dereferenceable pointer to a not freed object). And then you actually do want to check that they are within the committed boundaries of the heap. Otherwise you have a bug. We don't want to make such reasonable assertions weaker, because we cater for asserts that want oops to look valid pretty much all the time, even when they are stale pointers from a previous cycle, pointing into freed memory (until it is fixed). And I would much rather cater for asserts with good underlying reasoning (the oop should actually be okay here or something is wrong), than to cater for asserts that seem to just have the wrong underlying reasoning, making everyone else suffer for it, and letting bugs slip past all the other asserts. The is_in_reserved check no longer exists. And I don't think I want to bring it back from the dead for this assert. Because I don't think the assert is right. The GCs already have their own verification code for roots (and the heap), that does this job, but much better. So I don't think it makes sense for the actual shared code oop iterator to go in and put constraints on what an invalid (not yet fixed up) stale pointer should and should not look like, for all GC implementations. Let the GC's own verification code take care of that. It has a much better idea about what a stale pointer should and should not look like. And I do agree that we should not use barriers in this kind of code path. I just didn't know what else to do here to make the assert happy. Sorry about that. Well now I know. I am going to delete it, unless anyone opposes that solution. ------------- PR: https://git.openjdk.java.net/jdk/pull/296
