RFR 8042668: GC Support for shared heap ranges in CDS (RE: JDK-8059092)

[Tom Benson]

Hi Thomas,

I had said:
 > I want to look a little more at options for re-factoring the code 
that walks the
 > heap region array.  I'll post a separate reply about that.

I'm testing a version that I think satisfies your requests.  I did add a 
utility method to
HeapRegionManager, as you suggested, which accepts a MemRegion and 
allocates
all of the G1 regions that constitute the range.  It returns the count 
of regions that
required expansion, for the ergo_verbose reporting.  So that eliminated 
one walk in
alloc_archive_regions.

The other traversals now use addr_to_region, and advance through the 
range using
_hrm.next_region_in_heap , which both already existed.

So I was able to get rid of HeapRegionManager::addr_to_index and at_or_null.

I also got rid of G1BiasedArray:get_index_by_address, which was used to 
get indices for
setting a range using set_by_index in a loop.  Instead, I added an alternate
G1BiasedArray::set_by_address which accepts a MemRegion and sets the 
value for
all entries within that range.   So the loop is in the BiasedArray code 
itself.

I'll post an updated webrev when I've finished testing the changes.
Thanks,
Tom


On 6/4/2015 4:09 PM, Tom Benson wrote:
> Hi Thomas,
> Thanks for the review.   I've made many of the changes, except where 
> noted.
> I want to look a little more at options for re-factoring the code that 
> walks the
> heap region array.  I'll post a separate reply about that.
>
> > - Comments about that humongous regions are never added to the 
> collection
> > set in collectionSetChooser.hpp need to be updated. While it is still
> > true, that humongous regions are never added, we actually never want to
> > add pinned regions, which are a superset.
> > E.g. collectionsetChooser.hpp:106
>
> This comment was, in fact, updated to include archive regions, but 
> perhaps
> the parenthetical note made it hard to notice.  The review version says:
>
>  106   // not. Currently, we skip humongous regions (we never add them to
>  107   // the CSet, we only reclaim them during cleanup) and archive 
> regions,
>  108   // which are both "pinned", and regions whose live bytes are 
> over the
>  109   // threshold.
>
> So, I've now re-ordered it to make it more obvious.
>
>
> > - In general it would be nice if the asserts gave more information than
> > just some "this failed" message. E.g. it would be nice to at least 
> print the
> > index of the HeapRegion or the base address to be able to find out more
> > information about it just using the hs_err file.
> >
> > All changed asserts have that problem, so I am not mentioning them
> > explicitly.
>
> OK.  I focused on the ones I have added, rather than existing ones 
> where I may
> have changed "is_humongous" to "is_pinned", for example.  But I did 
> change the
> existing one in g1EvacFailure where the previous message was just 
> "sanity".
>
> I had already changed some in g1Allocator at Bengt's suggestion since 
> the version
> you reviewed.  Now I've changed others where I think it makes sense.
>
>
> > - G1Allocator::reuse_retained_old_region(): the comment talks about
> > humongous regions that need to be released. I think that it should talk
> > about pinned regions, and at the moment I cannot see a reason to keep
> > the allocation region if it has been made pinned in the meantime too.
> > This would mean that random objects could be added into pinned regions
> > while I believe it is best to specifically allocate into pinned 
> regions,
> > at least for now.
> >
> > I.e. the check to throw away the current allocation region should use
> > is_pinned(), not is_humongous().
>
> A region used by a G1Allocator should never be marked 'pinned' except 
> when it
> is humongous, so archive regions (the only other "pinned" regions) 
> should never
> wind up in reuse_retained_old_region.  There is an assertion just 
> above the
> comment you mentioned that the region is not an archive region.
>
> I could change the test from is_humongous() to is_pinned(), but that 
> would imply
> there is support for non-humongous pinned regions in G1Allocators.
>
>
> > - g1Allocator.cpp:189: if you expect an empty region, I would prefer 
> that
> > the corresponding getter from HeapRegion is used instead some ad-hoc
> > test (which is correct of course, but things may change for some or
> > another reason). I.e. HeapRegion::is_free().
>
> OK.
>
>
> > - G1Allocator::record_mem_allocate():
> >   - line 219: ... insert a filler...
>
> OK.
>
>
> >  - I think the code in line 225-231 may cause crashes/asserts.
> >
> > 225   if ((new_top > _max) ||
> > 226       ((new_top < _max) && (remainder < 
> CollectedHeap::min_fill_size()))) {
> > 227     if (old_top != _max) {
> > 228       size_t fill_size = _max - old_top;
> > 229       CollectedHeap::fill_with_object(old_top, fill_size);
> > 230       _summary_bytes_used += fill_size * HeapWordSize;
> > 231     }
> >
> > i.e. consider the situation exactly when the remaining space is too 
> small to
> > fit a filler object, i.e. old_top == _max - remainder
> > (where remainder == (G1CollectedHeap::min_fill_size() - 1)). In this 
> case,
> > the condition in line 226 holds (new_top < _max), and the remainder 
> is too
> > small to fit. So the predicate in 227 is trivially true, and we call
> > CollectedHeap::fill_with_object(). Am I overlooking something here?
>
> I think what you're overlooking is that "remainder" is not the remaining
> space in the region's current state, but what the remainder WOULD BE 
> after the
> requested allocation.  If the remainder will be too small, we don't do 
> the
> current allocation, and instead fill from the old_top to _max. That 
> size will
> always be greater than min_fill_size.
>
>
> >  - I would prefer if the method did not preemptively allocate a new 
> region,
> > but only if requested. Now it can happen that we end up with a 
> completely
> > empty region after the last string just filled up the previous one. 
> (If I
> > understand correctly)
>
> That shouldn't happen. There are 2 calls to alloc_new_region - one is 
> at the
> start of archive_mem_allocate, if there is no current 
> _allocation_region. The other
> is in the code that decides that the current requested allocation 
> doesn't fit in
> the current region.  In either case, we will do an allocation in the 
> region
> immediately, so it isn't "preemptive".
>
>
> > In G1RecordingAllocator::alloc_new_region():
> >
> > - In Hotspot-code memory ranges are typically right-open, i.e. do not
> > include the last address. G1MarkSweep::mark_range_archive() neither 
> does
> > that, nor is it documented. (Or use a base address + size, or 
> MemRegion).
>
> OK.  It now takes (HeapWord*, size_t) .
>
>
> > - Possibly put G1RecordingAllocator into its own files. I am not 
> insisting
> > on that.
>
> OK, I'll take you up on your offer to not insist.  8^)
>
>
> > - g1Allocator.cpp:224,228: please use pointer_delta() when calculating
> >  differences of addresses to avoid potential overflows.
>
> OK.  I see a couple of others below that as well, and one in 
> g1CollectedHeap.cpp.
>
>
> > G1RecordingAllocator::complete_recording():
> >   - the end_alignment parameter is not documented to be in bytes 
> (and in
> > general, it would be nice to mention the requirements that are checked
> > later). Also please use size_t for alignments, just in case. (There 
> does
> > not seem to be an overriding performance issue).
>
> OK, it is now "size_t end_alignment_in_bytes" , and the matching change
> has been made in g1CollectedHeap::end_archive_alloc_range.
>
>
> >  - check/guarantee() that end_alignment is aligned to HeapWordSize;
> >   please give actual/expected values in the error message.
> >  - line 262: please use align_pointer_up() instead of the hand-crafted
> >   (correct) formula.
> >  - line 264: please use pointer_delta()
>
> OK to all.
>
>
> >   - CollectedHeap::fill_with_objects() will assert if the given
> >    fill_size is < min_fill_size() -> bug
>
> Yes!  Fixed.
>
>
> >  - there is no need for the single "return;" statement at the end of
> >  the method
> >  - line 279: I would prefer if the decrement of the index using the --
> >  postfix operator on index would be put in an extra line. It is 
> surprising,
> >  and there does not seem to be a particular need here.
>
> OK to both.
>
>
> >  - I think the method still returns two memory ranges even if the
> >  space is actually contiguous. I would not expect that.
>
> Of course we've tested this change, and that doesn't happen. You get 1 
> MemRegion
> for ranges that are truly contiguous.  Normally the last G1 region 
> allocated will
> not be full, however, and since it is below the previously allocated 
> ones,
> there will be a gap.  I considered 'filling' the gap if it is smaller 
> than
> some threshold and just returning 1, but since CDS must be prepared to
> receive 2 anyway, it doesn't seem worthwhile.
>
>
> > G1RecordingAllocator::G1RecordingAllocator():
> >   - please consider initializing _bottom/_top/_max in the 
> constructor to
> > improve the likelihood of "clean" crashes if the API is used in the 
> wrong way.
>
> OK.
>
>
> > g1BiasedArray.hpp:
> >   - the change should not add specialized methods returning data of a
> >  particular type to the G1BiasedMappedArray base class that is used 
> only
> >  useful for a single instance.
> >  G1BiasedMappedArray is basically an abstraction of an array, and 
> indexes
> >  should always be of type idx_t (size_t), not uint.
> >  It is very dangerous if other users start using this method and
> >  potentially get a truncated value out of it (by not looking at the
> >  signature in detail).
> >  The change should either cast the value in the user, or add its own
> >  getter (if possible).
>
> OK. I had been swayed by region array indices being uint throughout
> heapRegionManager.  Now changed to idx_t.
>
>
> > G1CollectedHeap::is_record_alloc_too_large():
> >  - I would prefer if the comments were talking about the "why" for
> >  a given statement instead of just repeating the statement. I.e.
> >  instead of "Check whether the size would be considered humongous for
> >  a minimum-sized region.", it would be much more useful to write
> >  "We do not support humongous sized strings" or similar. Actually,
> >  it would be perfectly good if the method had a single description
> >  in the header file instead of putting almost the same comment
> >  twice here and in line 926f. Same with the comment at 939, which
> >  is more about "what" is done, not why.
>
> Yes, I agree 'why' is better here.  It's a rather long story in this 
> case,
> but I've summarized it to:
>   // Allocations in archive regions cannot be of a size that would be 
> considered
>   // humongous even for a minimum-sized region, because G1 region 
> sizes/boundaries
>   // may be different at archive-restore time.
>
> RE: line 939, I think the "what" here is useful, that the MemRegion array
> will be filled in by complete_archive.  But I don't feel strongly 
> about it,
> and will remove it if you do.
>
>
> > g1CollectedHeap::fill_with_non_humongous_objects()
> >  - any reason why the increment is half the humonguous threshold
> > and not humongous threshold-1 words?
>
> It was because I didn't want to have to worry about the increment
> plus min_fill_size being humongous, for the last fill after exiting the
> loop.  Using (humongous_threshold - (min_fill_size+1)) for the 
> increment would
> probably also work, but honestly, I don't think it matters much.
>
>
> > G1CollectedHeap::check_archive_addresses()
> >  - not sure why it uses an uint for a size specification. It seems
> > okay, but if there is no particular reason, I would prefer to use
> > size_t for sizes.
>
> I don't recall a compelling reason.  I'll change it.
>
>
> >  - is that method actually used anywhere? Grepping the sources I
> >  could not find any use for this method.
>
> This is used in the CDS code, so that the error of having mismatched heap
> addresses can be reported separately from a general allocation failure.
>
>
> > G1CollectedHeap::end_record_alloc_range():
> >   - why is end_alignment a uint, and not a size_t? Most if not all
> > other alignments for various purposes are size_t.
>
> OK.
>
>
> > G1CollectedHeap::alloc_archive_regions():
> >   - if the given count is zero, there is some wasted work, and we
> >  enable the slow path. Maybe guarantee that count > 0 and ranges
> >  != NULL?
>
> I added asserts to the check/alloc/fill entries.
>
>
> >  - I think this is the method where check_archive_addresses should
> >   be called?
>
> It's called by CDS before calling this.  However, I have added 
> 'guarantees'
> to alloc_archive_regions that check the addresses against heap boundaries
> and that they are in ascending order.
>
>
> >  - I would prefer if more guarantee()'s are used in that code
> >  instead of assert. It does not seem harmful in a performance
> >  point of view, but at least gives the fuzzy feeling that we
> >  get an immediate crash for invalid input data.
>
> The existing and new checks in alloc_archive_region are now 'guarantee.'
> The same checks are in fill_archive_region, but those are left as 
> asserts.
> The reason being that in the event the archive was corrupt, there 
> could be
> a bad range that would be handed to alloc_.  But once it has succesfully
> gotten through alloc_, it will be ok for fill_, unless CDS init does 
> something
> wrong internally. I just like to avoid needless code bloat in the 
> release version.
>
>
> >   - the count parameter should be size_t imo, just because it
> >   is a size of the given array.
> >   - 994: the comment should probably read "... in ascending
> >   (start) address order..."
>
> OK to both.
>
>
> >   - I think the code is buggy if the first region of the given
> >  memory range is region zero (which might happen). In that case,
> >  the code does not reserve region zero it seems, because line 1013
> >  increments the start_index.
>
> Region zero can't be part of the range, in this use.  We start allocating
> at the high end of heap, right after JVM init.  However, it would also
> work to initialize prev_end_index to something which is not a valid 
> index,
> like the last valid index + 1 , so I've changed the code to do that.
>
>
> >   - in line 1017, I think the comment needs to be fixed: "available"
> > already has a distinct meaning in heap region management (in this
> > code). See below.
>
> OK.
>
>
> >   - please avoid assignment and check in the condition if not
> >  necessary for some reason (the "if ((curr_region = ...) == NULL)").
> >  The current code is just hard to read, particularly because it
> >  does not save any code size, ie.
> >
> >  1020       HeapRegion* curr_region;
> >  1021       if ((curr_region = _hrm.at_or_null(curr_index)) == NULL) {
> >
> >  seems much worse to read than
> >
> >  1020       HeapRegion* curr_region = _hrm.at_or_null(curr_index);
> >  1021       if (curr_region == NULL) {
> >
> >  Since curr_region is needed in the outer scope anyway (in the
> >  else-part), I do not see a good reason to put the assignment in
> >  the if-statement.
>
> I don't either.  I think it wound up that way through evolution of 
> different
> versions. I've changed it.
>
>
> >   - the code from 1017 to 1035 seems to be best placed as some
> > utility method in HeapRegionManager. HeapRegionManager::at_or_null()
> > leaks details about internal region organization to G1CollectedHeap.
> > It would also allow iteration over the "heap region array", which is
> > an implementation detail of HeapRegionManager (which is, what the
> > most recent huge changes to region management explicitly tried to
> > make impossible).
> >   - HeapRegionManager::addr_to_index(): the code should use the
> > HeapRegion*'s hrm_index() getter to retrieve the region's index if
> > required. The HeapRegionManager should expose as little as possible
> > details, including that the regions are consecutive. Not sure about
> > this. I mean it's nice to have, but not necessary at all.
> > Removing it would (I think) also remove the need for the ugly change
> > in G1BiasedMappedArray.
>
> I think the re-factoring suggested here will take a little more thought.
> Part of it could be done entirely in HeapRegionManager, but I think some
> needs to be in G1CollectHeap.  I think this may generate more discussion,
> so I'll post a separate reply.
>
>
> >  G1CollectedHeap::fill_archive_regions():
> >   - the code should check the passed ranges for minimal validity,
> > e.g. use check_archive_addresses(). That method might also check
> > for ascending addresses in the list - at least fill_archive_regions()
> > seems to miss it. Please also put such requirements in the parameter
> > description.
>
> As noted above, I added checks.
>
>
> >  - (also for other methods): please make sure to use the exact
> >  terminology for variable names that is used elsewhere, i.e. for
> >  start/top/end/last/bottom. Several variables are called "end_XYZ",
> >  but contain the "last" address. Making the naming uniform improves
> >  readability and avoids constant lookup up of their exact meaning.
> >  Please also try to avoid new words like "base" for no apparent
> >  reason. Also, the code might be slightly less complicated (as in
> >  less characters to be parsed and understood) if "end" were used
> >  instead of "last", which would save some "+1" calculations and
> >  "<=" in comparisons (using "<" for the latter).
>
> OK, I tried to make that more consistent.
>
>
> >  - please rename "mr" to "reserved", as this is more specific.
>
> OK.
>
>
> >   - line 1068   // that contain the address range.  The address
> >  range actually within the : I think the "actually" is misplaced,
> >  maybe should go at the beginning of the sentence.
>
> No, I meant that the address range which is actually represented by
> the MemRegion will not be modified.  Filler objects might be added
> in the G1 region that contains that address range, if necessary, to make
> it parseable.
>
>
> >  - I personally would prefer if "word_size" were defined more
> >  closely to its use. I.e. in this case, there are 30 lines
> >  between definition and use.
>
> OK.  Removed the local.
>
>
> >  - I think fill_archive_regions() has the same bug as
> > alloc_archive_regions is the first region in the memory range is
> > region zero.
>
> Not a bug, but I made the same change.
>
>
> >   - fill_with_non_humongous_objects() should not be called if
> >  fill_size is < min_fill_size(), as it asserts in debug mode.
>
> Since the archive allocator won't leave a space smaller than 
> min_fill_size
> at the top of an allocation region, this won't arise.
>
>
> > VerifyArchiveOopClosure::do_oop_work():
> >   - "Archive object references a non-pinned object" - isn't it that
> >  archived objects can only reference other archived objects, not
> >  just pinned? I might be wrong...
> >    - same in the comment for VerifyArchiveRegionClosure::do_object(),
> > and VerifyRegionClosure::doHeapRegion()
>
> Yes, text strings are wrong, code is right.  Fixed.
>
>
> > G1CollectedHeap::is_obj_dead_cond():
> >   - not sure if it is performance critical, but the changed code
> >  now always evaluates the region type.
>
> Yes, but these are only used in heap verification.  I could have
> used G1MarkSweep::in_archive_range(obj) instead, but the
> HeapRegion* was already at hand.
>
>
> > G1CollectedHeap::rebuild_region_sets():
> >   - the code clears the summary_used field of the recording_allocator
> > here. Where is its value recreated? I actually do not think this is
> > required: the contents of the archive regions never change anyway.
> > It should be doable to create a test that first creates a shared
> > archive, then uses it in the next VM invocation, and checks whether
> > summary bytes are okay after full gc/evacuation failure (see below)?
> >
> > G1CollectedHeap::do_collection_pause_at_safepoint(), line 4101:
> >   - why clear the used bytes for the recording allocator since my
> >  understanding is that archive regions will never be collected?
>
> The _allocator->set_used calls at both of the above points 'inherit' the
> space allocated by the ArchiveAllocator.  EG, recalculate_used walks the
> regions and adds up the used size.  It seemed best to me not to perturb
> the _allocator's idea of how much heap was in use, even if some of it
> had originally been allocated by the ArchiveAllocator.  If the
> ArchiveAllocator's total is not cleared, verification fails because the
> space is double counted.
>
> I think this is really moot at present, since the G1ArchiveAllocator
> is created and and destroyed in a single safepoint, after which the JVM
> exits.  But I tested scenarios where it was allowed to stay around.
>
>
> > g1CollectedHeap.cpp:
> >   - please avoid the double-spaces between sentences in comments. At
> >  least in this file, the majority of comments do not use two spaces
> >  after a full stop.
> >   - line 3368: extra spaces before the brace
>
> OK.
>
>
> > G1CollectedHeap.hpp:
> >   - I would prefer if methods were commented separately about what
> > they do and what they expect instead of one big blob of comments
> > before a group of methods. This leads to unnecessary searching if just
> > wanting to find out about parameter usage. Also this increases the
> > likelihood if one method is changed, the developer does not update
> > the corresponding comment lots of lines above. Do not misunderstand,
> > I am fine with an introductory/summary comment about a group of 
> methods,
> > just not that it contains the descriptions of the method signatures,
> > like in the comment in line 735 and 748. Please do not abbreviate
> > method names in comments.
>
> OK. I personally find it more readable and more strongly connecting
> the routines the way it is, but I'll change it to your preferred form.
>
>
> >  - line " 737   // end address and returns the allocated ranges as
> > an ascending array" - what is an ascending array?
> >  738   // of MemRegions.  This can be used to create and archive a heap
> >  739   // region which can be mapped at the same fixed addresses in 
> a future
> >  740   // VM instance.
> >
> > I think it should read ... create and archive a set of heap regions
> > which...
> >
> > Maybe it could be clarified what a "future VM instance" is.
>
> OK.
>
>
> > g1MarkSweep.cpp
> >   - line 61: extra space between G1ArchiveRegionMap and G1MarkSweep::
> >   - extra newlines in line 330,331
>
> OK.
>
>
> > G1SpaceCompactClosure::doHeapRegion(): I think any pinned regions 
> should
> > be skipped for compaction, not only archived.
>
> I used is_archive because Humongous regions are already 
> conditionalized out above,
> so using "!is_pinned" could be misleading.  Same thing for 
> G1AdjustPointersClosure
> and G1AdjustPointersClosure.  However, I don't feel strongly about it, 
> so have
> changed them.
>
>
> > G1MarkSweep::enable_archive_object_check():
> >   - indentation in the call to initialize()
> >   - the cast in front of HeapRegion::Grainbytes is superfluous
> >
> >  321     _archive_region_map.set_by_index(index++, true);
> >
> > using the ++ operator within the argument seems unnecessary here, just
> > makes the statement harder to read.
>
> OK to all.
>
>
> > G1PrepareCompactClosure::doHeapRegion():
> >   - again I think any pinned regions should be skipped here.
>
> Answered above.
>
>
> > g1MarkSweep.hpp:
> >
> > Please add a comment to what G1ArchiveRegionMap does/is supposed to do.
> >
> >   58   // Support for 'archive' objects, to prevent objects in 
> archive regions
> >   59   // from being marked by full GCs.
> >
> > "Support for XYZ" seems a bit too sparse.
> >
> > Please at least document the parameters for mark_range_archive(),
> > whether the parameters are inclusive/exclusive, etc. Or pass a
> > MemRegion, because it has defined semantics.
>
> OK, I've bulked up the comments.  The mark_range_archive arguments are an
> address and length, not 2 addresses, so I don't think 'exclusive' is 
> really
> necessary.
>
>
> > G1VerifyLiveClosure::do_oop_work()
> >   - why is the remembered set for pinned regions not checked? We do 
> need
> > the remembered set if it gets unpinned.
>
> This conditional was already there for Humongous regions.  For archive
> regions, there is no way for them to become 'unpinned.'  If that
> is a future goal, I think a follow-on project/testing is called for.
>
>
> > heapRegion.hpp:421:
> >   - please add a little comment about what pinned/archive means and the
> > difference.
>
> OK.
>
>
> > heapRegionManager.cpp:
> >
> > HeapRegionManager::find_highest_available():
> >   - the code will never return or consider the region zero for use. 
> I am
> > not sure this is intended.
>
> Yes, it was not viewed as an issue, knowing the usage.  But it's 
> easily changed,
> and I did so.
>
>
> >   - please document that the method will automatically commit any
> > previously uncommitted regions in the header file, not in the cpp file.
> > I mean, the comment in the cpp file would be very good start (except 
> for
> > the "loop downwards", and comments below) for the documentation of the
> > interface.
> > At least from the method name it would not have occurred to me that it
> > also commits the region. Also, "available" has a specific meaning in 
> the
> > context of HeapRegionManager, i.e. that the region can be allocated 
> into
> > (either empty or already allocated into).
> > Also, the documentation does not say anything about what happens when
> > there is no suitable region.
>
> OK.  I changed the comments and renamed it find_highest_free().
>
>
> > HeapRegionManager.hpp:
> >   - HeapRegionManager::addr_to_index(): please use the HeapRegion*'s
> > hrm_index() getter to retrieve the region's index. It is an 
> implementation
> > detail that the index in the _regions table of HeapRegionManager
> > corresponds to the HeapRegion's index.
> >   - HeapRegionmanager::at_or_null(uint index) const; should be removed,
> > see above.
>
> I'll defer this to the 'how to traverse regions' message I said
> I would send separately.
>
>
> >   - HeapRegionManager::find_highest_available() method: please put 
> input
> > parameter descriptions in the header file, not in the implementation.
>
> OK.
>
>
> > heapRegionManager.inline.hpp.
> >  - HeapRegionManager::addr_to_index(): not sure if there is a point to
> > remind you that the lower levels already do the asserting, and have two
> > levels of brackets with a more cryptic than explaining comment ("which
> > might be outside the current heap") here.
>
> The comment was there to tell the reader that the address range is indeed
> being checked for validity, a question I would expect. But I'll remove 
> it.
>
>
> > > Performance:  The GC changes had no significant impact on SpecJBB,
> >
> > For Specjbb2005, did you check object copy/young gc times specifically?
> > They do not do a lot of GCs in the first place, so an overall 
> performance
> > regression would require a very large performance regression in 
> young gc
> > times to be noticed.
>
> I only specifically checked full GC times for the reasons I mentioned, 
> relying
> on impact to benchmarks to show other impact.  I can look at that as 
> well with
> the new version.
>
>
> > I still need to go through all is_humongous() checks to see if 
> something
> > has been missed for pinning/archiving support.
>
> Yes, that's exactly what I did.
>
>
> Thanks,
> Tom
>