RFR: 8080775: Better argument formatting for assert() and friends
Per Liden
per.liden at oracle.com
Thu Sep 24 12:56:08 UTC 2015
Hi David,
On 2015-09-24 10:21, David Lindholm wrote:
> Kim,
>
> Thanks again for looking at this. I have fixed your comments below, and
> even though you have some files left I have uploaded a new webrev with
> the changes I have made so far:
>
> http://cr.openjdk.java.net/~david/JDK-8080775/webrev.01/
> http://cr.openjdk.java.net/~david/JDK-8080775/webrev.00-01/ (differential)
>
This is a really nice cleanup! Just a few comments:
g1AllocRegion.inline.hpp:
-------------------------
Could we rename G1_ALLOC_REGION_ASSERT() to something like
assert_alloc_region(), to match the style used by assert_heap_locked()
and friends in g1CollectedHeap.hpp.
heapRegionSet.hpp/heapRegionSet.cpp:
------------------------------------
It feels like we should get rid of hrs_err_msg and hrs_ext_msg here
also, and replace it with an assert_heap_region_set() or something, or
is there a reason why we're keeping those?
vmError.cpp:
------------
VMError::_current_step could be a local variable in report() instead of
a static.
VMError::_current_step_info could be reset to "" in the END macro,
instead of outside of report().
VMError::_verbose could be an argument to report() instead of a static.
cheers,
/Per
> I will also file a new RFE to fix src/share/vm/gc/g1/heapRegionSet.cpp
> per your suggestion.
>
> Thanks,
> David
>
>
> On 2015-09-23 20:32, Kim Barrett wrote:
>> On Sep 22, 2015, at 7:38 AM, David Lindholm
>> <david.lindholm at oracle.com> wrote:
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8080775
>>> Webrev: http://cr.openjdk.java.net/~david/JDK-8080775/webrev.00/
>> Another batch, this time covering gc through memory. I need to take a
>> break from this, but I should be able to finish tomorrow.
>>
>> Mostly formatting issues. BTW, although I'm finding some of these, I'm
>> also noticing that you tidied up a lot more. Thanks for that.
>>
>> ------------------------------------------------------------------------------
>>
>> src/share/vm/gc/cms/compactibleFreeListSpace.cpp
>> 2877 assert(n * word_sz == fc->size(),
>> 2878 "Chunk size " SIZE_FORMAT " is not exactly splittable by "
>> 2879 SIZE_FORMAT " sized chunks of size " SIZE_FORMAT,
>> 2880 fc->size(), n, word_sz);
>>
>> Pre-existing: Indentation of message arguments.
>>
>> ------------------------------------------------------------------------------
>>
>> src/share/vm/gc/g1/bufferingOopClosure.cpp
>> 157 assert((got) ==
>> (expected), \
>> 158 "Expected: %d, got: %d, when running testCount(%d,
>> %d, %d)", \
>> 159 (got), (expected), num_narrow, num_full, do_oop_order)
>>
>> 194 assert((got) ==
>> (expected), \
>> 195 "Expected: %d, got: %d. testIsBufferEmptyOrFull(%d,
>> %d, %s, %s)", \
>> 196 (got), (expected), num_narrow,
>> num_full, \
>> 197 BOOL_TO_STR(expect_empty), BOOL_TO_STR(expect_full))
>>
>> 234 assert(boc.is_buffer_empty(),
>> 235 "Should be empty after call to done().
>> testEmptyAfterDone(%d, %d)",
>> 236 num_narrow, num_full);
>>
>> Pre-existing: Indentation of message arguments.
>>
>> ------------------------------------------------------------------------------
>>
>> src/share/vm/gc/g1/g1AllocRegion.hpp
>> 37 #define G1_ALLOC_REGION_MSG(message) \
>> 38 "[%s] %s c: %u b: %s r: " PTR_FORMAT " u: " SIZE_FORMAT, \
>> 39 _name, message, _count, BOOL_TO_STR(_bot_updates), \
>> 40 p2i(_alloc_region), _used_bytes_before
>>
>> Misaligned line continuations.
>>
>> I think this doesn't belong in this header; it's not something that a
>> client can use. Suggest moving it to g1AllocRegion.inline.hpp.
>>
>> Suggest changing this to G1_ALLOC_REGION_ASSERT, with predicate
>> argument and full assert expression as expansion. Macros with unusual
>> syntactic expansions can be confusing, so best avoided unless they
>> provide some benefit that can't be obtained in some other way.
>>
>> ------------------------------------------------------------------------------
>>
>> src/share/vm/gc/g1/g1AllocRegion.hpp
>> 49 class G1AllocRegion VALUE_OBJ_CLASS_SPEC {
>> 50 friend class ar_ext_msg;
>>
>> ar_ext_msg no longer exists.
>>
>> ------------------------------------------------------------------------------
>>
>> src/share/vm/gc/g1/g1Allocator.cpp
>> 432 assert((end_alignment_in_bytes >> LogHeapWordSize) <
>> HeapRegion::min_region_size_in_words(),
>> 433 "alignment " SIZE_FORMAT " too large",
>> end_alignment_in_bytes);
>>
>> Pre-existing: Indentation of message arguments.
>>
>> ------------------------------------------------------------------------------
>>
>> src/share/vm/gc/g1/g1BiasedArray.cpp
>> 44 guarantee(biased_index >= bias() && biased_index < (bias() +
>> length()),
>> 45 "Biased index out of bounds, index: " SIZE_FORMAT " bias: "
>> SIZE_FORMAT " length: " SIZE_FORMAT, biased_index, bias(), length());
>>
>> 50 guarantee(biased_index >= bias() && biased_index <= (bias() +
>> length()),
>> 51 "Biased index out of inclusive bounds, index: " SIZE_FORMAT
>> " bias: " SIZE_FORMAT " length: " SIZE_FORMAT, biased_index, bias(),
>> length());
>>
>> Pre-existing: Indentation of message arguments.
>>
>> Maybe also split really long lines?
>>
>> ------------------------------------------------------------------------------
>>
>> src/share/vm/gc/g1/g1BlockOffsetTable.cpp
>> 364 assert((_array->offset_array(orig_index) == 0 &&
>> 365 blk_start == boundary) ||
>> 366 (_array->offset_array(orig_index) > 0 &&
>> 367 _array->offset_array(orig_index) <= N_words),
>>
>> [Pre-existing, and not modified by this changeset, but nearby.]
>>
>> Indentation of 366 and 367 are wrong.
>>
>> ------------------------------------------------------------------------------
>>
>> src/share/vm/gc/g1/g1BlockOffsetTable.hpp
>> 151 assert(offset <= N_words,
>> 152 "%s - "
>> 153 "offset: " SIZE_FORMAT ", N_words: %u",
>> 154 msg, offset, (uint)N_words);
>>
>> Maybe eliminate the line break in the format string?
>>
>> ------------------------------------------------------------------------------
>>
>> src/share/vm/gc/g1/g1BlockOffsetTable.inline.hpp
>> 95 assert(result >= _reserved.start() && result < _reserved.end(),
>> 96 "bad address from index result " PTR_FORMAT
>> 97 " _reserved.start() " PTR_FORMAT " _reserved.end() "
>> 98 PTR_FORMAT,
>> 99 p2i(result), p2i(_reserved.start()),
>> p2i(_reserved.end()));
>>
>> Maybe eliminate the second line break in the format string?
>>
>> ------------------------------------------------------------------------------
>>
>> src/share/vm/gc/g1/g1CollectedHeap.cpp
>> 2540 assert(_old_marking_cycles_started ==
>> _old_marking_cycles_completed ||
>> 2541 _old_marking_cycles_started == _old_marking_cycles_completed
>> + 1,
>> 2542 "Wrong marking cycle count (started: %d, completed: %d)",
>> 2543 _old_marking_cycles_started, _old_marking_cycles_completed);
>>
>> Pre-existing: Indentation of arguments.
>>
>> ------------------------------------------------------------------------------
>>
>> src/share/vm/gc/g1/heapRegionSet.cpp
>>
>> Suggest eliminating hrs_ext_msg and hrs_err_msg in a manner similar to
>> elimination of ar_ext_msg, taking into account earlier comments in
>> that area.
>>
>> Please do that under a new RFE; this change set is quite larg enough
>> already.
>>
>> ------------------------------------------------------------------------------
>>
>> src/share/vm/gc/g1/heapRegionType.hpp
>> 30 #define hrt_assert_is_valid(tag) \
>> 31 assert(is_valid((tag)), "invalid HR type: %u", (uint) (tag))
>>
>> Pre-existing: I don't think this should be a macro. I suggest it
>> should be a private member function using NOT_DEBUG_RETURN and an
>> ASSERT-only definition in the .cpp.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8137046
>>
>> ------------------------------------------------------------------------------
>>
>> src/share/vm/gc/parallel/mutableNUMASpace.cpp
>> 87 assert(words_to_fill >= CollectedHeap::min_fill_size(),
>> 88 "Remaining size (" SIZE_FORMAT ") is too small to
>> fill (based on " SIZE_FORMAT " and " SIZE_FORMAT ")",
>> 89 words_to_fill, words_left_to_fill,
>> CollectedHeap::filler_array_max_size());
>>
>> Pre-existing: Indentation of message arguments.
>>
>> ------------------------------------------------------------------------------
>>
>> src/share/vm/gc/shared/gcCause.hpp
>> 97 assert(cause !=
>> GCCause::_old_generation_too_full_to_scavenge &&
>> 98 cause != GCCause::_old_generation_expanded_on_last_scavenge,
>> 99 "This GCCause may be correct but is not expected yet: %s",
>> 100 to_string(cause));
>>
>> Pre-existing: Indentation of arguments.
>>
>> ------------------------------------------------------------------------------
>>
>> src/share/vm/gc/shared/space.cpp
>> 606 assert(oop(last)->is_oop(),PTR_FORMAT " should be an object
>> start", p2i(last));
>>
>> Add space between comma and PTR_FORMAT.
>>
>> ------------------------------------------------------------------------------
>>
>>
>
More information about the hotspot-dev
mailing list