RFR: 8080775: Better argument formatting for assert() and friends
Per Liden
per.liden at oracle.com
Fri Sep 25 06:15:10 UTC 2015
Hi,
On 2015-09-24 16:25, David Lindholm wrote:
> Hi Per,
>
> Thanks for the review! Comments inline.
>
>
> On 2015-09-24 14:56, Per Liden wrote:
>> This is a really nice cleanup! Just a few comments:
>
> Thanks!
>
>>
>> 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.
>
> Fixed.
>
>> 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?
>
> Yes, per Kims suggestion I will add another RFE to fix this in another
> changeset.
>
>> vmError.cpp:
>> ------------
>> VMError::_current_step could be a local variable in report() instead
>> of a static.
>
> Yes, but the local variable must still be static, otherwise
> crashreporting during crashreporting won't work.
Ah, ok. In that case I think it's better to leave next to
current_step_info, since that are tightly connected to each other.
cheers,
/Per
>
>> VMError::_current_step_info could be reset to "" in the END macro,
>> instead of outside of report().
>
> Yes, I reset both current_step_info and current_step in END now.
>
>> VMError::_verbose could be an argument to report() instead of a static.
>
> Fixed.
>
>
> Thanks,
> David
>
>>
>> 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