[webkit-reviews] review denied: [Bug 174566] [WTF] Drop WTFGetBacktrace and WTFPrintBacktrace : [Attachment 315781] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied  review:
Bug 174566: [WTF] Drop WTFGetBacktrace and WTFPrintBacktrace
https://bugs.webkit.org/show_bug.cgi?id=174566

Attachment 315781: Patch

https://bugs.webkit.org/attachment.cgi?id=315781&action=review




--- Comment #9 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 315781
  --> https://bugs.webkit.org/attachment.cgi?id=315781
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315781&action=review

I'm going to r- this.  I think it's worth taking another look after all the
changes has been made.	Thanks.

> Source/WTF/wtf/StackTrace.cpp:59
> +    numberOfFrames = GET_BACK_TRACE(&m_skippedFrame0, numberOfFrames);

Let's add StackTrace::InternalFramesToSkip (see below) to numberOfFrames and
framesToSkip before this.

>> Source/WTF/wtf/StackTrace.cpp:70
>> +static ALWAYS_INLINE size_t instanceSize(int capacity)
> 
> ALWAYS_INLINE seems like overkill here. Just a normal inline should be fine,
and in fact this is even a simple enough function that the compiler would
decide to inline it even if we didn’t specify inline. We normally use
ALWAYS_INLINE only when there is a serious reason to force inlining, such as a
critical performance dependency or something else like that.

Let's rename capacity to requestedFramesToCapture.

> Source/WTF/wtf/StackTrace.cpp:74
> +    // StackTrace includes first two skipped frames.
> +    ASSERT(capacity >= StackTrace::SkippedFrames);
> +    return sizeof(StackTrace) + (capacity - StackTrace::SkippedFrames) *
sizeof(void*);

Let's rename StackTrace::SkippedFrames to StackTrace::InternalFramesToSkip.

I think we can simplify this not relying on the client to add
StackTrace::InternalFramesToSkip.  Instead, let's just add it here.  This is
why I suggest renaming capacity to requestedFramesToCapture.  It does not
include StackTrace::InternalFramesToSkip, and therefore, we don't need this
assert anymore.  And the equation reduces to: sizeof(StackTrace) +
requestedFramesToCapture * sizeof(void*);

>> Source/WTF/wtf/StackTrace.cpp:80
>> +	ASSERT(framesToSkip >= 0);
> 
> The need for these assertions shows us that our interfaces should be using
"unsigned" rather than "int".

I agree that it would be nice if we can switch to using unsigned instead.

> Source/WTF/wtf/StackTrace.cpp:82
> +    framesToSkip += StackTrace::SkippedFrames;

Remove this and the comment above it.

>> Source/WTF/wtf/StackTrace.cpp:85
>>	std::unique_ptr<StackTrace> trace(new (NotNull,
fastMalloc(sizeToAllocate)) StackTrace());
> 
> No need for the "()" after "StackTrace".
> 
> Not new to this patch, but I don’t see any code that would make the delete
operator use fastFree rather than free when deallocating one of these.

It will be fastFreed because the StackTrace class is WTF_MAKE_FAST_ALLOCATED. 
The purpose of the use of placement new here is not to select fastMalloc as the
allocator, but to control the size of the allocation to add space for the
captured trace.

> Source/WTF/wtf/StackTrace.cpp:96
> +    // Skip 2 additional frames i.e. FixedStackTraceBase and
StackTrace::captureCurrentStack.
> +    framesToSkip += StackTrace::SkippedFrames;

Remove these.

>> Source/WTF/wtf/StackTrace.h:39
>> +	static const constexpr int SkippedFrames { 2 };
> 
> I’m surprised that we need both const and constexpr: I’m not enough of an
expert on constexpr to know what the correct idiom is. For an int does
constexpr do anything different from const?
> 
> Would be a little more elegant if this constant could be private rather than
public because it’s not part of the interface to this class. That might require
moving the static_assert inside a member function, but really I think that
would be good. The static_assert could go inside
StackTrace::captureCurrentStack right next to the code that depends on it. If
we did that, then instanceSize would be need to also be a private static member
function.
> 
> It’s confusing to use the almost identical names "skipped frames" and "frames
to skip" for 5 or 6 different things:
> 
>     1) Number of frames we always need to skip in StackTrace.
>     2a) Number of additional skipped frames requested by the caller.
>     2b) Number of additional skipped frames passed as a FixedStackTrace
template argument.
>     3) Total number of frames we need to skip: (1) + (2).
>     4) Storage in the StackTrace union to guarantee we have space for (1) we
always need to skip.
>     5) Storage for additional skipped frames in FixedStackTrace to give use
space for (2).
> 
> I think we should have five different names. Here is my cut at suggestions
(add "m_" and capitalize as appropriate):
> 
>     1) baseFramesToSkip or internalFramesToSkip
>     2) additionalFramesToSkip or just framesToSkip
>     3) totalFramesToSkip
>     4) internalSkippedFrameStorage
>     5) additionalSkippedFrameStorage or just skippedFrameStorage

Let's rename SkippedFrames to InternalFramesToSkip.  Also add the comment here
that these frames are StackTrace::capture and StackTrace::captureCurrentStack.

>> Source/WTF/wtf/StackTrace.h:85
>>	    };
> 
> Seems like this should be an array of size SkippedFrames rather than two
separate members. That would make the static_assert less necessary.
> 
>     void* m_skippedFrames[SkippedFrames];

This is a great idea.

>>> Source/WTF/wtf/StackTrace.h:88
>>> +static_assert(sizeof(StackTrace) == sizeof(void*) *
StackTrace::SkippedFrames, "StackTrace members should be placed in skipped 2
frames.");
>> 
>> I don’t understand what "placed in skipped" means here. There seems to be a
wording problem.
> 
> Here is the comment I would write:
> 
>     "StackTrace object needs to be big enough so we can temporarily overwrite
it with 2 skipped frames during capture."

Let's rename StackTrace::SkippedFrames to StackTrace::InternalFramesToSkip.