Parallel other tests #6150

saschanaz · 2018-01-28T11:32:00Z

Tried implementing what @juj suggested:

I think the solution forward here might be to assign unique/dedicated temp directories to each parallel test runner process in core and other? That is, for each parallel test runner thread, append a unique subdirectory path to EMCC_TEMP_DIR for each of them, so that they all operate in unique directories? Then the leak checker could also be enabled for the core suite.

Edit: other now only takes 16 minutes on CircleCI (while ~40 minutes on Travis)!

kripken

Overall I like the result here, but I'm a little confused on how this changes the status of the temp dirs, what the meaning of the canonical one is, and where that is set.

kripken · 2018-01-29T17:24:43Z

.circleci/config.yml

-    environment:
-      - TEST_TARGET=other.test_n* other.test_o* other.test_p* other.test_q* other.test_r* other.test_s* other.test_t* other.test_u* other.test_v* other.test_w* other.test_x* other.test_y* other.test_z* skip:other.test_native_link_error_message
-      # TODO: remove skips after fastcomp update on travis for 1.37.23
+      - TEST_TARGET=other skip:other.test_bad_triple skip:other.test_native_link_error_message skip:other.test_emcc_v skip:other.test_failing_alloc


why is emcc_v skipped?

It has been skipped since its birth because it tends to fail whenever an SDK version update happens: #5883

kripken · 2018-01-29T17:26:00Z

emcc.py

-      os.makedirs(temp_root)
-    temp_dir = tempfile.mkdtemp(dir=temp_root)
+  temp_root = shared.TEMP_DIR
+  if not os.path.exists(temp_root):


creating shared.TEMP_DIR could be done in shared, I think? seems cleaner that way, unless I'm missing something, and we don't want shared to always create it?

Some tools including test runners, emar, emconfigure, etc. also imports shared, we probably don't want to create and remove empty directory for those tools.

Good point, yes.

kripken · 2018-01-29T17:28:27Z

tests/test_other.py

+
  def clean_emcc_files_in_temp_dir(self):
-    for x in os.listdir(CANONICAL_TEMP_DIR):
+    for x in os.listdir(self.canonical_temp_dir):


self.canonical_temp_dir is defined as a function in runner.py, but used as a string here?

Ah, the class clean_write_access_to_canonical_temp_dir has its own string field canonical_temp_dir.

kripken · 2018-01-29T17:29:21Z

tests/test_other.py


  def test_emcc_debug_files(self):
    if os.environ.get('EMCC_DEBUG'): return self.skip('cannot run in debug mode')
+    canonical_temp_dir = self.canonical_temp_dir()


this seems confusing. do we need a function there, can't it always be a string?

but a larger question, why is the test runner in charge of declaring the canonical temp dir? shouldn't shared.py do that?

Hmm, it can be a string. I'll fix it.

This PR dynamically assigns temporary directory path for each test runner thread so that each thread won't get any conflict when writing files in its canonical temp directory. This is not needed for normal emcc use, and shared.py is not related to parallel tests, so this is done in test runners.

I see. I'm not sure, but I think we can get that benefit (each test is independent) by cleaning up the shared.py temp dir code, and using EMCC_TEMP_DIR or one of those. That seems better as the cleanup would be good by itself, and we wouldn't need any new code in the test harness for it.

As I said I'm not sure it can work, and it's a different approach than this PR, so I'd understand if you're not interested to investigate it, I could do that.

kripken · 2018-01-29T17:30:18Z

tests/runner.py

      return ('-O2' in self.emcc_args or '-O3' in self.emcc_args or '-Oz' in self.emcc_args) and not (Settings.SIDE_MODULE or Settings.BINARYEN)

+  def canonical_temp_dir(self):
+    return os.path.join(self.temp_dir, 'emscripten_temp')


we still have logic in shared.py that uses that string "emscripten_temp", can we share that code so it's not duplicated?

kripken · 2018-01-30T21:39:10Z

Ok, this looks good to me. I think we could probably simplify the test code some more (but I'm not sure - I might look into it if I have time), but my bigger concerns before were with the non-test code, and all that looks great.

kripken · 2018-02-16T23:42:34Z

@saschanaz Very strange, when I run other.test_os_oz now on incoming, I get

======================================================================
ERROR: test_os_oz (test_other.other)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/alon/Dev/emscripten/tests/test_other.py", line 3288, in test_os_oz
    with clean_write_access_to_canonical_temp_dir(self.canonical_temp_dir):
AttributeError: 'other' object has no attribute 'canonical_temp_dir'

----------------------------------------------------------------------
Ran 1 test in 0.022s

FAILED (errors=1)

Bisection lead me to here, but it's odd we didn't notice this breakage before. Any ideas?

It should be fine for all parallel tests to using the same TEMP_DIR. The only limitation is on tests that use the canonical test directory (e.g. EMCC_DEBUG). It looks like we used to use a unique temp directory per process (See emscripten-core#6150). However that we changes to one-directory-per-test in emscripten-core#18214, although I don't remember why. With this change we don't create a new temp directory except for tests that are marked as `uses_canonical_tmp`

saschanaz added 8 commits January 28, 2018 20:24

minor refactor

9906174

thread dedicated temp dir

006c118

allow parallel other tests

7b61f77

merge again

68cfcec

move EMCC_TEMP_DIR to shared.py

8c8facd

explicit EMCC_TEMP_DIR

eca3356

dynamic canonical temp dir

3dcb39a

skip test_failing_alloc on circleci

91f5200

saschanaz force-pushed the other-parallel2 branch from 6fcbab2 to 91f5200 Compare January 28, 2018 14:30

kripken reviewed Jan 29, 2018

View reviewed changes

set_temp_dir

4fd49bc

kripken merged commit 4594b09 into emscripten-core:incoming Jan 30, 2018

saschanaz deleted the other-parallel2 branch January 30, 2018 23:44

sbc100 mentioned this pull request Oct 21, 2025

Avoid using a new temp directory for each parallel test. NFC #25615

Open

Uh oh!

Parallel other tests #6150

Parallel other tests #6150

Uh oh!

Conversation

saschanaz commented Jan 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saschanaz Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saschanaz Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Jan 30, 2018

Uh oh!

kripken commented Feb 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

saschanaz commented Jan 28, 2018 •

edited

Loading

saschanaz Jan 30, 2018 •

edited

Loading

saschanaz Jan 30, 2018 •

edited

Loading