perf_hooks: remove useless calls in Histogram

[mhdawson]

Coverity reported some calls that had no effect,
remove them

Signed-off-by: Michael Dawson [email protected]

[mhdawson]

I guess this might explain why the call was made ? On OSX:

../src/histogram.cc:162:5: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
map->Set(
^~~~~~~~

[mhdawson]

Not sure why it did not fail locally, as seems to be reported for linux as well. Maybe my local compiler version?

[mhdawson]

I think I understand what is going on here:

• Set returns MaybeLocal and has V8_WARN_UNUSED_RESULT -> which results in a warning if the return value from Set is not used.
• IsEmpty() is one of the few methods on MaybeLocal that does not also have a V8_WARN_UNUSED_RESULT. Since it is marked const however, coverity knows that it does do anything other than satisfying the compiler.

Our options would seem to be:

• use ToLocalChecked instead of IsEmpty as it is not const and does not have V8_WARN_UNUSED_RESULT
• add a co-verity comment telling it to ignore the line.

[mhdawson]

ToLocalChecked will crash if the result is empty so we'd only use that if we want to know that the Set failed versus just ignoring and continuing.

[mhdawson]

No local test failures with ToLocalChecked so I think that means it's not expected that the Set can fail.

[mhdawson]

@jasnell since I think you are the main contributor to this part of the code do you think I should:

• use ToLocalChecked instead of IsEmpty as it is not const and does not have V8_WARN_UNUSED_RESULT
• add a co-verity comment telling it to ignore the line.

Some additional explanation/discussion above.

[RaisinTen]

This would crash if an exception is thrown between subsequent V8 calls in any of these binding functions. As a solution, instead of removing the IsEmpty() checks, we should pass on the return value of the Set() call from the lambdas and in

node/src/histogram-inl.h

Lines 61 to 70 in 22792c8

	template <typename Iterator>
	void Histogram::Percentiles(Iterator&& fn) {
	Mutex::ScopedLock lock(mutex_);
	hdr_iter iter;
	hdr_iter_percentile_init(&iter, histogram_.get(), 1);
	while (hdr_iter_next(&iter)) {
	double key = iter.specifics.percentiles.percentile;
	fn(key, iter.value);
	}
	}

while calling fn(), we should check if the returned value is an empty maybe and return early if that's the case.

[addaleax]

+1 to what @RaisinTen said.

If that's not an option, we should use USE(x) instead of x.IsEmpty(), which is literally there for the very purpose of avoiding "unused value" warnings.

[jasnell]

I suggest just using USE(...) here as Anna mentioned.

[mhdawson]

Thanks, I'll look at @RaisinTen's suggestion and if that does not seem practical add the USE()

[mhdawson]

Code higher up looks like

 get percentilesBigInt() {
    if (!isHistogram(this))
      throw new ERR_INVALID_THIS('Histogram');
    this[kMap].clear();
    this[kHandle]?.percentilesBigInt(this[kMap]);
    return this[kMap];
  }

[mhdawson]

Returning the result of map-Set up to the JavaScript would only let us return undefined/null instead of an empty map, but I think this[kHandle]?.percentilesBigInt(this[kMap]); may already lead to the possibility of an empty map in case of an error/invalid state.

For that reason I think just using USE makes sense, will update with that.

[richardlau]

Commit message will need updating to reflect the new approach.

[mhdawson]

@RaisinTen pushed update to change to add USE()

[RaisinTen]

LGTM

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/42190/

[mhdawson]

Landed in d86dcaa