Re: [PATCH v3 1/5] MdeModulePkg/UefiBootManagerLib: fix LoadImage/StartImage status code rep.

[Laszlo Ersek <lersek@redhat.com>]

On 02/25/19 09:27, Ni, Ray wrote:
> On 2/23/2019 1:16 AM, Laszlo Ersek wrote:
>> Yes, I was fully aware of that.
>>
>> However:
>>
>> The issue is that, in the BmReportLoadFailure() function, we do some
>> work*before*  we call REPORT_STATUS_CODE_EX(). We have an ASSERT(), a
>> ZeroMem(), and a field assignment.
>>
>> If status code reporting is disabled for EFI_ERROR_CODE in the platform,
>> then said work will be wasted. We can optimize this by checking for
>> ReportErrorCodeEnabled() up-front, because we know for sure that later
>> on we will report the status code with EFI_ERROR_CODE type.
>>
>> In other words, this approach is similar to DEBUG_CODE(). In some cases,
>> logging a piece of information with DEBUG() takes non-trivial
>> computation. And it would be a waste, for example in RELEASE builds, to
>> perform the computation, and then throw away only the result (the log
>> message). Therefore the DEBUG_CODE macro is used, and the whole work is
>> eliminated in RELEASE builds.
>>
>> The idea is the same here. If the compiler can statically deduce that
>> ReportErrorCodeEnabled() will always return FALSE -- for example because
>> the ReportStatusCodeLib instance in question looks at
>> "PcdReportStatusCodePropertyMask", and the PCD is Fixed-at-Build, and
>> the corresponding bit is clear --, then the compiler can eliminate the
>> entire BmReportLoadFailure() function. This is good for both flash usage
>> and for performance.
>>
>> I'm fine either way, but first, please confirm again that you really
>> want me to remove the ReportErrorCodeEnabled() check, before pushing.
>>
>> Thanks!
>> Laszlo
> 
> Thanks for the explanation. I am fine with both.
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> 

Thank you, Ray. I opted for pushing v3 as-is.

Laszlo