benchmark: remove %OptimizeFunctionOnNextCall

[bzoz]

Checklist

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

Affected core subsystem(s)

benchmark

Description of change

This removes all instances of %OptimizeFunctionOnNextCall from Node.js benchmarks. As it turns out, most bechmark benefit from its removal - they will perform better. See this table in gist.

Some of the benchmarks (buffers/buffer-swap.js, crypto/get-chiper.js, net/punnycode.js, path/parse-*.js, path/relative-*.js and tls/convertprotocols.js) benefit from warmup phase. Those are executed twice with only second run being measured. For other benchmarks warmup does not provide any advantage.

One benchmark that is slower is crypto/get-chiper.js, when calling getCiphers once. Previous version called this function once to trigger optimizations then to benchmark its performance. Results of that function are cached, so it didn’t provide valid data. This is fixed now.

Fixes: nodejs/node-chakracore#134
cc @nodejs/benchmarking

[evanlucas]

/cc @mscdex since he's done a lot of performance work too

[mscdex]

I think this should only be removed for functions that are potentially inlineable due to their function size.

[bzoz]

@mscdex For most of the benchmarks except ones mentioned in PR removing the %OptimizeFunctionOnNextCall improves the performance. Shouldn't we at least remove it for those?

Could you also elaborate on those inlineable functions?

[bzoz]

Rebased on master, PTAL

[jasnell]

@mscdex ... any further thoughts on this?

[mscdex]

As far as inlineability I would just check the current function lengths (there are other factors that determine inlineability of course). If the function being tested is less than 600, remove the forced optimization, otherwise leave it there.

[bzoz]

Most of the time %OptimizeFunctionOnNextCall is used for either small functions or functions from node.js lib. Is there any benefit to have this %Optimize..?

If we remove it, it will make some of the benchmark perform better. Furthermore, this will make benchmarks engine agnostic, so they can be used with chakracore (see nodejs/node-chakracore#134)

[fhinkel]

I'm pretty sure that inlineability does not depend on function length any more but bytecode size (at least in the near future).

[fhinkel]

I'm in favor of making the benchmarks vm agnostic. I don't see the point in measuring only the optimized runs instead of all the runs.

[trevnorris]

I'm pretty sure that inlineability does not depend on function length any more but bytecode size

As of 5.7.201 (and is the same on a6976211d1a4d12df815fa9ff5341dcbcf1e8036, latest master) https://github.com/v8/v8/blob/5.7.201/src/flag-definitions.h#L329-L330

[trevnorris]

I don't see the point in measuring only the optimized runs instead of all the runs.

The idea is to remove variability in the benchmarks to better compare between versions. Ideally we could run the benchmarks fully optimized and run them not allowing to optimize the code at all.

[bzoz]

@trevnorris So, are you in favor of this change? Or maybe you have other suggestions?

[bzoz]

Rebased, PTAL

[bzoz]

Rebased again, PTAL

[bzoz]

/cc @nodejs/collaborators

[jasnell]

@mscdex ... Side note: I've been wondering if it would be worthwhile to introduce linting for functions that are over the inlinable size. Sure, there are quite a few instances of fns in core over that size, and I'm not suggesting that we should refactor those, just that they should require linting exceptions.

[jasnell]

@trevnorris ... I've had a todo on my list for a while to see if there was some way that we can build in checking optimized and non-optimized run's for all benchmarks. I definitely think it would be worthwhile.

Likewise, we should likely be making it easier to run the benchmarks in a way that compares use of the ignition toolchain vs. crankshaft.

[addaleax]

@jasnell Won’t that go away with Turbofan anyway (v8/v8@0702ea3)?

[jasnell]

Yes, sorry, I wasn't clear... it would be useful to be able to more easily run compare.js with various combinations of command line args... e.g. something like a ./node benchmark/compare.js --old "node" --new "node --ignition" stream ...

(if this already exists and I just missed it, then just ignore me ;-) ...)

[Trott]

Refocusing the conversation: I imagine @bzoz wants to know if anyone feels good enough about this to give it an approval or else to describe clearly what changes would need to happen to get your approval. Anyone?

[jasnell]

While I'm generally ok with the idea of removing the forced optimization, I'd rather it be done more incrementally than this... with either one benchmark or one related group of benchmarks edited per commit.

[evanlucas]

This isn't landing cleanly on v7.x-staging. Mind submitting a backport PR?

[bzoz]

@evanlucas backport here: #11744