lib: cache length prop in classic for loop

[SukkaW]

Cache length prop in classic for loop for performance purpose.

Refs: #30958

---

There is a common view that the modern javascript engine will perform the optimization in for (;) loop (classic for loop). But according to my own practices, cache length prop manually is still at least 7% (mostly 15%) faster.

According to #30958 (review), some for (;) is retained for performance concern, IMHO adding such a "optimization" should be accepted then.

FYI, I have noticed that lib/_http_outgoing.js already uses the same syntax when I bring up the PR:

node/lib/_http_outgoing.js

Lines 242 to 245 in 30cc542

	for(leti=0,l=keys.length;i<l;i++){
	constkey=keys[i];
	headers[headersMap[key][0]]=headersMap[key][1];
	}

Checklist

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

[himself65]

https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/640

[SukkaW]

https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/640

05:35:48 + git clone https://github.com/nodejs/benchmarking.git 05:35:48 Cloning into 'benchmarking'... 05:40:27 error: RPC failed; curl 56 GnuTLS recv error (-9): A TLS packet with unexpected length was received. 05:40:27 fatal: The remote end hung up unexpectedly 05:40:27 fatal: early EOF 05:40:27 fatal: index-pack failed 

It appears that the CI failed to clone nodejs/benchmarking.

cc @himself65

[himself65]

https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/640

05:35:48 + git clone https://github.com/nodejs/benchmarking.git 05:35:48 Cloning into 'benchmarking'... 05:40:27 error: RPC failed; curl 56 GnuTLS recv error (-9): A TLS packet with unexpected length was received. 05:40:27 fatal: The remote end hung up unexpectedly 05:40:27 fatal: early EOF 05:40:27 fatal: index-pack failed 

It appears that the CI failed to clone nodejs/benchmarking.

cc @himself65

Yes, I see. restart new one

[mscdex]

V8 has optimized the existing style (not caching array length) for quite some time now and last I checked/heard caching the length can actually prevent such optimizations from happening.

Additionally I would be very surprised if these changes showed up in http benchmarks because other things like network I/O and the http parser are going to dominate the majority of the time spent.

[SukkaW]

@mscdex

... last I checked/heard caching the length can actually prevent such optimizations from happening.

As I said, according to my own practices, cache length prop manually is still at least 7% (mostly 15%) faster, and Node.js itself utilize the syntax as well. But I am really curious about benchmark results.

I would be very surprised if these changes showed up in http benchmarks because other things like network I/O and the http parser are going to dominate the majority of the time spent.

See #30958 (review). All header manipulations and socket operations are considered as performance sensitive.

[mscdex]

and Node.js itself utilize the syntax as well

The reason for that is that there was a time when caching the length did improve performance in general, but that was a long time ago.

[SukkaW]

did improve performance in general ... but that was a long time ago.

It is known to all that most modern JavaScript Engines apply optimization in that way. But in most cases, cache length prop is still faster (even it breaks the "optimization").

In the end, only the benchmark will tell the answer.

[mscdex]

@himself65 I think you'll probably want to stop the http benchmark because running all of the http cases with all of the possible configurations for each will take weeks or more, which means no other benchmarks can run during that time.

I suggest manually selecting the minimal set of files in benchmarks/http (that cover the http changes being made in this PR) to run and for each of them explicitly set configuration parameters (IIRC in CI this can be done by passing the desired --set foo=bar arguments in the "Filter" field after the filter value) to reduce the number of configurations that will execute for that file. This is especially important because some combinations of parameters are actually redundant (e.g. different chunk lengths when chunked encoding is disabled) and just waste time.

[himself65]

@mscdex
sorry for I previously not realize the time costs. I will take care about the minimal set of benchmark :)

[github-actions]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

[jasnell]

Closing due to lack of progress.