Request for review (XS): 7030435: Some oop_oop_iterate_m() methods iterate outside of specified memory bounds

[Y. Srinivas Ramakrishna]

A  quick fyi: i just synced with hotspot-gc
(after ~ a week?), and it turns out that Tom has
already made some of these changes (for the case
of arrayKlassKlass and objArrayKlassKlass) in a recent
putback:-

http://hg.openjdk.java.net/jdk7/hotspot-gc/hotspot/rev/c7f3d0b4570f

  So the only real payload (XXS now :-) that this
changeset will contain will be the change to constantPoolKlass.

-- ramki

On 3/24/2011 12:10 PM, Y. Srinivas Ramakrishna wrote:
> John Coomes brought up the important question
> of safety of this change in that we might either
> scan too little now, or were scanning too much before.
> I thought i'd share the discussion here, and I'd also
> promised to follow up on (2) below (in terms of checking
> whether those field updates did card-marks).
>
> (1) does scanning outside of the mem-interval cause
> issues with closures that do not expect to do so:
> besides the newly modified verification closure,
> there are scavenging closures that expect that no
> old-to-new reference is scanned more than once.
> However, because in this case all of the objects
> involved are meta-data in perm gen, we are safe
> since these are not cross-generational into the
> young gen.
>
> (2) does not scanning outside the mem-interval now
> cause not enough oops to be marked? Recall that
> the memory-range-delimited iterators are used currently
> only for card-scanning (either for cross-generational
> pointers or, as used by CMS, in the form of an incremental
> update barrier that adds to a modified reference set).
> For the cross-generational reference case, we are fine
> because as we noted above the targets are in the same
> perm-space (at least as of today).
> If these fields are updated by means of an oop_store()
> we would be fine even if these were cross-space pointers.
> It turns out that all of these field updates use either
> oop_store() (so safe even in the cross-space case), or
> they use oop_store_wirthout_check(). The latter would be
> unsafe for cross-space pointers, but is safe for incremental
> update barriers that collect modified references because
> in those cases we set AlwaysDoUpdateBarrier which does
> a card-mark. (If and when some of those using the _without_check()
> form move to a different space, they might need the _with_check()
> form for safety, should they need to update remembered set
> information to allow independent collection.)
>
> In summary, the changes are safe today for both our stop-world
> and for our concurrent CMS collector. However, it's also
> true, as I noted in the CR, that we didn't really have
> a correctness issue before; may be perhaps a negligible
> theoretical (but not real) performance issue wrt sometimes
> over-scanning by a wee bit. However, fixing the iterators
> so they stay within the interval specified beings them in
> line with expectations, intention and specification,
> and makes for safety and hygiene of client code as it
> evolves.
>
> Many thanks again to everyone for their reviews, and i'll make
> the modifications stemming from the reviews before pushing
> these changes.
> -- ramki
>
> On 3/24/2011 1:39 AM, Y. Srinivas Ramakrishna wrote:
>>
>> This fixes the issues found by the newly added
>> assert in the card-table verification closure, apropos
>> of my last code review request.
>>
>> Tested in the same manner as described in the code
>> review request for 7029036 that i sent a little while
>> ago.
>>
>> webrev: http://cr.openjdk.java.net/~ysr/7030435/webrev.00/
>>
>> Many thanks for your reviews!
>> -- ramki
>