1305268 - Replace some MOZ_ASSERTs with MOZ_DIAGNOSTIC_ASSERTs in memory/.

Status: RESOLVED WONTFIX

(Core :: Memory Allocator, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: MOZ_DIAGNOSTIC_ASSERT_memory.patch

Nick, your thread on dev-platform promoting MOZ_RELEASE_ASSERT and MOZ_DIAGNOSTIC_ASSERT motivated me to pursue an idea I had already been thinking about: replace cheap MOZ_ASSERTs (e.g. simple expressions without function calls, not in hot inline functions) with MOZ_DIAGNOSTIC_ASSERTs to increase assertion testing from Nightly and DevEdition users.

I started with the memory/ directory because if you are not supportive of this approach, then I won't bother trying to convert other directories to MOZ_DIAGNOSTIC_ASSERT.

Comment 1

[Nicholas Nethercote [inactive]]

Comment on attachment 8794557 [details] [diff] [review]
MOZ_DIAGNOSTIC_ASSERT_memory.patch

Review of attachment 8794557 [details] [diff] [review]:
-----------------------------------------------------------------

I definitely appreciate the attempt to improve our quality via extra feedback from real users. But I'm not sure this is the best way to do it.

For one, I'd like to know the effect on binary size. You've only added a few extra asserts here, but if we added enough it might make a significant difference. Measurements would be informative.

Second, some of the supposedly non-hot assertions actually are hot. I've indicated which ones below.

Third, currently when you see MOZ_DIAGNOSTIC_ASSERT you know that it's an unusually important one, in some sense. Making this change en masse would muddy that.

I suspect adding more diagnostic asserts is probably best done by people familiar with the code in question? Feel free to ask for a second opinion...

::: memory/replace/dmd/DMD.cpp
@@ +471,5 @@
>  
>    void Lock()
>    {
>      MutexBase::Lock();
> +    MOZ_DIAGNOSTIC_ASSERT(!mIsLocked);

This is hot.

@@ +477,5 @@
>    }
>  
>    void Unlock()
>    {
> +    MOZ_DIAGNOSTIC_ASSERT(mIsLocked);

This is hot.

@@ +708,5 @@
>  
>    uint32_t Length() const { return mLength; }
>    const void* Pc(uint32_t i) const
>    {
> +    MOZ_DIAGNOSTIC_ASSERT(i < mLength);

Hot.

@@ +744,3 @@
>      st->mPcs[st->mLength] = aPc;
>      st->mLength++;
> +    MOZ_DIAGNOSTIC_ASSERT(st->mLength == aFrameNumber);

Both hot.

::: memory/replace/dmd/test/SmokeDMD.cpp
@@ +179,5 @@
>    // Analyze 1: reported.
>    // Analyze 2: freed, irrelevant.
>    char* e3 = (char*) realloc(nullptr, 1023);
>  //e3 = (char*) realloc(e3, 0);
> +  MOZ_DIAGNOSTIC_ASSERT(e3);

Given that this is a test, MOZ_RELEASE_ASSERT is probably clearer.

Comment 2

[Chris Peterson [:cpeterson]]

Thanks for your feedback. I don't know the impact on code size, especially if many directories where converted to MOZ_DIAGNOSTIC_ASSERT. I'll withdraw this bug and leave the more precision use of MOZ_DIAGNOSTIC_ASSERT to people working in each module.

I won't bother submitting a new change for test/SmokeDMD.cpp because the assertion is just for realloc() failure, which is highly unlikely in this test and would the following code would crash anyways.