Fix benchmark issues

[mscdex]

Checklist

• make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
• the commit message follows commit guidelines

Affected core subsystem(s)

• benchmark

Description of change

These commits fix some issues that exist since c570182 and 83432bf.

[mscdex]

/cc @AndreasMadsen

[AndreasMadsen]

Can you explain the motivation behind not using parseFloat? It is such a burden to make sure all benchmarks works and performs the correct amount of iterations.

[mscdex]

IIRC Originally I did not realize that numeric strings were being automatically converted because most benchmarks were already doing conversions and it tripped me up when I was trying to benchmark the http changes from the same PR. Specifically one of the functions being benchmarked checks the type and returns early if it's not a string. So when I started running that benchmark, I was getting unexpected results.

IMHO keeping all of the values as strings makes things more consistent and the changes here are small compared to the total number of benchmarks.

[AndreasMadsen]

I did not realize that numeric strings were being automatically converted

Perhaps we should just JSON.stringify and JSON.parse the values instead.

I think the current implementation (never parsing) is confusing because we allow numbers as input, but converts them to strings. To ensure consistency we would have to only allow strings, which would be rather inconvenient.

[mscdex]

@AndreasMadsen I've added the missing stringify in the code you referenced. I think allowing numbers in the default values is fine as it shows the intention better.

[AndreasMadsen]

I think allowing numbers in the default values is fine as it shows the intention better.

I agree. But setting number in common.createBenchmark and getting strings out in main is not obvious and I believe it will only lead to confusion. To be clear, I don't think there is a good solution that involves never parsing the input arguments.

What I'm sugesting is that we use JSON.stringify and JSON.parse to avoid the problem of numeric strings being parsed, but still allow numbers.

[mscdex]

@AndreasMadsen If that were implemented it'd mean you'd have to start using escaped double quotes to force a number to be treated as a string which could be equally "confusing", for example:

node bench.js foo=\"5\"

[mscdex]

/cc @nodejs/collaborators

[AndreasMadsen]

@mscdex I can understand that it is inconvenient, but you will have to explain how it is "equally confusing".

Another solution could be to transfer the configuration parameters with process.send and then only use the variable=value for manually specifying a parameter. In that case I don't think it is too important how the value part is parsed.

[trevnorris]

IMO using + for string parsing is the correct way of doing it. It coerces the value the same way as would a numeric comparison e.g. 5 < '10' and handles all supported number types correctly e.g. ' 0xff'. Since we don't care about the decimal, parseInt() may be a suitable alternative.

[AndreasMadsen]

@trevnorris We can replace parseFloat with +, that is fine. But it doesn't solve the original issue, that numeric strings are converted to numbers when they should just be strings.

[trevnorris]

@AndreasMadsen wouldn't each parameter know whether it should be a string or a number?

[AndreasMadsen]

@trevnorris A parameter could have both string and number values, but apparently there doesn't currently exists such a case.

[trevnorris]

@AndreasMadsen Since it doesn't exist yet can we implement a rule that it can't. :) I can't think of a good reason for doing that, when it wouldn't make more sense to simply have two difference parameters.

[AndreasMadsen]

Since it doesn't exist yet can we implement a rule that it can't.

@trevnorris Sounds like a good idea.

I can't think of a good reason for doing that, when it wouldn't make more sense to simply have two difference parameters.

A case where different types of values is used is in the assert benchmark, but is uses strings to identify the actual value.

[mscdex]

/cc @nodejs/collaborators Any more input on this? We already have two other open PRs that fix benchmarks fixed by this PR.

[Trott]

Not sure about the JSON.stringify() change (maybe it just needs some details added to the commit message) but the other changes all LGTM.

[mscdex]

@Trott The JSON.stringify() is necessary mostly for default parameter values that include control characters, such as \r or \n.

[Trott]

@mscdex Oh, I see. LGTM then.

[mcollina]

LGTM

[AndreasMadsen]

@mscdex I'm pretty sure the JSON.stringify commit is wrong.

Right now if a 'string' is passed to createBenchmark, it will be encoded as 'key="sstring"' and then decoded as '"sstring"' (there is no JSON.parse in the decoder).

I could be wrong (I can't check right now), but \r or \n shouldn't matter when they are parsed to spawn, because the arguments array specify the separation.

edit: based on our initial discussion that commit should just be removed.

[Trott]

@mscdex Maybe land the other two commits more-or-less right now, and put the stringify() commit in its own PR for discussion if you think it's needed? (On the other hand, if you land the stringify() commit and it's not quite right, it is trivial to revert or correct, so whatevs, but it would be best if you and @AndreasMadsen were in agreement on this, of course.)

[mscdex]

@AndreasMadsen @Trott I've removed that commit.

[Trott]

LGTM

[AndreasMadsen]

Considering the amount of issues related to this, I don't think we have the time to discuss this anymore. But I still think this is a bad fix that is only going to add confusion. I would much rather see that @trevnorris suggestion was implemented.

[Trott]

@AndreasMadsen What are you referring to as "a bad fix"? The use of stringify() (which isn't in the PR anymore)? Or the use of + to coerce? Or what?

I'm mostly interested in seeing the process.exit(0) stuff land.

[mscdex]

IMHO having less magic here is better. To me, it makes it easier to think about.

[AndreasMadsen]

It is "Or the use of + to coerce?". Right now number inputs to createBenchmark implicitly becomes strings in the main function.

The suggested fix is to infer the type from the options argument and add a rule such mixed types aren't allowed.

[AndreasMadsen]

For the record: the process.exit() commit landed in 8bb59fd, the suggested parsing method and type mixing check landed in f99471b.