test: only refresh tmpDir for tests that need it #1954

Trott · 2015-06-12T02:10:56Z

Expose common.refreshTmpDir() and only call it
for tests that use common.tmpDir or common.PIPE.

A positive side effect is the removal of a code
smell where child processes were detected by the
presence of .send(). Now each process can decide
for itself if it needs to refresh tmpDir.

/cc @rvagg (who suggested this approach in comments on #1877)

rvagg · 2015-06-12T04:25:43Z

for tests that use common.tmpDir or common.PIPE.

Note that common.PIPE isn't necessarily in the temporary directory - we set it to something completely different on all of the CI machines because of the character limit in path names for pipes and on Windows it's a named pipe so has nothing to do with the filesystem. So if you wanted to you could optimise even further! Up to you though.

I'd love to see some timing numbers for this, can you time a full test run with and without this change and post the results here?

brendanashworth · 2015-06-12T04:57:54Z

test/common.js

+  try {
+    rimrafSync(exports.tmpDir);
+  } catch (e) {
+  }


I think this usually goes on the same line } catch (e) { }, is the linter OK with that?

make jslint doesn't complain. There's one similar occurrence in the file (although it has a comment in the empty block so maybe that's different): https://github.com/nodejs/io.js/blob/a6b8ee19b85bbd798510191f0aee596f36b909d2/test/common.js#L159-L161

Hmm. I'd say the comment makes it a little bit different, but i'll leave it up to you if the linter is happy either way.

Trott · 2015-06-12T05:01:25Z

I'd love to see some timing numbers for this, can you time a full test run with and without this change and post the results here?

Do you mean something like time python tools/test.py --mode=release message parallel sequential -J a few times each with and without this optimization? Or is there a relatively easy way to get better numbers?

brendanashworth · 2015-06-12T05:07:18Z

@Trott probably for this, I'd think time python tools/test.py --mode=release parallel would be best. (but Rod is the build master so he knows better)

rvagg · 2015-06-13T01:46:42Z

yeah, just a time of this branch vs master would be interesting, doesn't need to be precise, just interesting to see what (if anything) this is buying us. Leave out parallel for now I think.

jbergstroem · 2015-06-13T03:47:13Z

Might as well try both

Trott · 2015-06-13T03:58:51Z

Not seeing a dramatic difference running time python tools/test.py --mode=release message sequential but there is something (although this is n of 1 so it could just be random variation).

master:

real    0m41.301s
user    0m29.730s
sys 0m4.569s

Branch in this PR:

real    0m40.614s
user    0m29.308s
sys 0m4.375s

rvagg · 2015-06-13T04:04:53Z

oh, that's disappointing! I'm supposed to send you Jenkins creds so when you get them you can run on the CI cluster and compare run times across platforms, if we get a big enough win on the ARM machines then this'd be worth it

Trott · 2015-06-13T06:18:19Z

On Jenkins, armv7-ubuntu-1404 takes 13 minutes with and without this change. No measurable change.

I suspect that will be the story across the board, despite my fervent hope that it shaves a few minutes of the Raspberry Pi test runs (which I guess we'll find out about in the next hour or so).

Jenkins runs against:

I'm still in favor of this change. It gets rid of the code smell where child processes were detected by the presence of .send(). I wasn't really comfortable with that one.

But I can see a counterargument that it introduces complexity compared to the previous "always refresh the tmpdir no matter what" approach.

rvagg · 2015-06-13T06:23:52Z

armv7-ubuntu-1404 may actually be quicker at fs operations than some of the other slaves because they have local eMMC storage, not virtualized like most of the other machines and not an SD card like the pi's.

Trott · 2015-06-13T06:26:42Z

Ah, OK. I'll hold out hope a bit.

I should probably be comparing test run times and not the whole time listed which will include the build time. I'm unfamiliar with Jenkins and not seeing an easy way to get a total time spent running tests. The best I've found is comparing times for individual tests in the TAP Extended Test Results. If there's an obvious better place for me to look, please clue me in.

rvagg · 2015-06-13T06:33:50Z

Yeah, this is a good point actually - some of the slave groups are backed by multiple machines and some of them use ccache so there's no guarantee you're going to get a similar build time! most of the standard linux slaves are backed by a single machine, all of the ARM except for the ubuntu1404 one are backed by multiples as is Windows. This might be a fruitless excercise UNLESS you push two branches that put a time in front of test.py execution so you get that on the output.

Expose `common.refreshTmpDir()` and only call it for tests that use common.tmpDir or common.PIPE. A positive side effect is the removal of a code smell where child processes were detected by the presence of `.send()`. Now each process can decide for itself if it needs to refresh tmpDir.

Trott · 2015-06-13T23:08:25Z

I followed your (@rvagg) suggestion to put time in the Makefile to see what results we get with the tmpdir refresh happening for every test vs. happening only for the tests that need it. Obviously, all the platforms that don't have a time command (or didn't have it installed for whatever reason) failed. But of the ones that succeeded, here's how it ended up.

As you can see in the table below, elapsed execution is faster (with one exception), but not always by a whole lot. The largest savings is ubuntu1404-64 shaving off 42 seconds. I have no idea what's up with ubuntu1404-32 which took 7 seconds longer. In both cases, I don't know how much of the change is ordinary variability. But I think it's probably significant that 9 out of 10 builds were faster and the one that wasn't was only a little bit slower.

Platform	Master Real	PR Real	Master User	PR User	Master Sys	PR Sys
freebsd101-32	309.68	308.58	197.77	197.30	18.80	18.32
freebsd101-64	308.40	308.06	186.25	185.25	27.82	28.04
osx1010	302.72	298.09	176.84	173.40	34.64	34.38
smartos14-32	7:32.7	7:16.2	3:24.1	3:21.0	2:05.4	2:03.5
smartos14-64	7:24.9	7:22.3	3:13.1	3:11.6	2:15.5	2:13.7
ubuntu1204-64	6:52.18	6:36.35	265.79	253.98	35.07	33.59
ubuntu1404-32	5:46.79	5:51.98	219.66	223.64	24.12	25.22
ubuntu1404-64	6:33.96	5:51.98	254.39	223.64	31.64	25.22
ubuntu1410-64	9:11.83	8:49.15	366.90	347.71	51.84	51.89
ubuntu1504-64	6:38.06	6:07.03	257.19	237.65	26.56	24.22

rvagg · 2015-06-14T01:04:59Z

thanks for the numbers, this lgtm I guess

jbergstroem · 2015-06-14T02:35:01Z

LGTM from me as well.

Expose `common.refreshTmpDir()` and only call it for tests that use common.tmpDir or common.PIPE. A positive side effect is the removal of a code smell where child processes were detected by the presence of `.send()`. Now each process can decide for itself if it needs to refresh tmpDir. PR-URL: #1954 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

Trott · 2015-06-14T06:02:30Z

Merged in 7c79490

mscdex added the test Issues and PRs related to the tests. label Jun 12, 2015

brendanashworth reviewed Jun 12, 2015
View reviewed changes

Trott force-pushed the export-tmpdir branch from 112eda5 to e4d05f9 Compare June 12, 2015 20:17

Trott force-pushed the export-tmpdir branch from e4d05f9 to fa6af5c Compare June 13, 2015 03:55

Trott force-pushed the export-tmpdir branch from fa6af5c to 7a5e712 Compare June 13, 2015 22:07

Trott closed this Jun 14, 2015

rvagg mentioned this pull request Jun 16, 2015

Release proposal: v2.3.1 #1996

Closed

Trott deleted the export-tmpdir branch January 9, 2022 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: only refresh tmpDir for tests that need it #1954

test: only refresh tmpDir for tests that need it #1954

Trott commented Jun 12, 2015

rvagg commented Jun 12, 2015

brendanashworth Jun 12, 2015

Trott Jun 12, 2015

brendanashworth Jun 12, 2015

Trott commented Jun 12, 2015

brendanashworth commented Jun 12, 2015

rvagg commented Jun 13, 2015

jbergstroem commented Jun 13, 2015

Trott commented Jun 13, 2015

rvagg commented Jun 13, 2015

Trott commented Jun 13, 2015

rvagg commented Jun 13, 2015

Trott commented Jun 13, 2015

rvagg commented Jun 13, 2015

Trott commented Jun 13, 2015

rvagg commented Jun 14, 2015

jbergstroem commented Jun 14, 2015

Trott commented Jun 14, 2015

test: only refresh tmpDir for tests that need it #1954

test: only refresh tmpDir for tests that need it #1954

Conversation

Trott commented Jun 12, 2015

rvagg commented Jun 12, 2015

brendanashworth Jun 12, 2015

Choose a reason for hiding this comment

Trott Jun 12, 2015

Choose a reason for hiding this comment

brendanashworth Jun 12, 2015

Choose a reason for hiding this comment

Trott commented Jun 12, 2015

brendanashworth commented Jun 12, 2015

rvagg commented Jun 13, 2015

jbergstroem commented Jun 13, 2015

Trott commented Jun 13, 2015

rvagg commented Jun 13, 2015

Trott commented Jun 13, 2015

rvagg commented Jun 13, 2015

Trott commented Jun 13, 2015

rvagg commented Jun 13, 2015

Trott commented Jun 13, 2015

rvagg commented Jun 14, 2015

jbergstroem commented Jun 14, 2015

Trott commented Jun 14, 2015