Run test262 tests in parallel #564

bnoordhuis · 2024-09-28T22:24:54Z

This commit introduces a couple of changes in order to make run-test262 go brr and execute tests in parallel:

Remove CONFIG_AGENT build option. The disabled version of the build was already broken and no one noticed, Remove the define altogether.
Remove the -C switch. Hard to support in multi-threaded mode. I may bring it back some day because it is useful.
Remove the -r switch. Also hard to support and I never look at test262_report.txt anyway so on the chopping block it goes.
Judicious use of thread-local storage so I don't have to thread through state everywhere and embiggen the diff even more.

This is what Real Programmers(TM) do: stay up coding way past midnight just so the test suite finishes in one minute instead of four.

Fixes: #547

This commit introduces a couple of changes in order to make run-test262 go brr and execute tests in parallel: - Remove CONFIG_AGENT build option. The disabled version of the build was already broken and no one noticed, Remove the define altogether. - Remove the -C switch. Hard to support in multi-threaded mode. I may bring it back some day because it _is_ useful. - Remove the -r switch. Also hard to support and I never look at test262_report.txt anyway so on the chopping block it goes. - Judicious use of thread-local storage so I don't have to thread through state everywhere and embiggen the diff even more. This is what Real Programmers(TM) do: stay up coding way past midnight just so the test suite finishes in one minute instead of four. Fixes: quickjs-ng#547

bnoordhuis · 2024-09-28T22:42:51Z

Right, tcc and gcc48 of course don't understand _Thread_local... I'll kill off those buildbots but I may take a stab at adding it to tcc.

bnoordhuis · 2024-09-28T22:59:35Z

Good news: TSan seems pleased with this version of run-test262

Bad news: TSan is waaaay too slow for CI, it takes literal CPU hours to churn through test262...

saghul

Cool! Left some quick notes, will do a more thorough review tonight!

saghul · 2024-09-29T06:03:24Z

.github/workflows/ci.yml

@@ -84,20 +83,6 @@ jobs:
        run: |
          sudo sysctl -w kernel.randomize_va_space=0

-      - name: install TCC


Any chance we can keep the removed CI by not compiling test262? The interpreter should work just fine...

Yes, good idea, done. Should've thought of that myself (but hey, way past midnight.)

saghul · 2024-09-29T06:59:21Z

run-test262.c

@@ -1268,13 +1296,7 @@ static int eval_buf(JSContext *ctx, const char *buf, size_t buf_len,
    if (JS_IsException(res_val)) {
        exception_val = JS_GetException(ctx);
        is_error = JS_IsError(ctx, exception_val);
-        /* XXX: should get the filename and line number */
-        if (outfile) {
-            if (!is_error)


Why not keep this part?

It's part of the -r switch (reportfile config option) that I removed.

saghul · 2024-09-29T07:04:22Z

run-test262.c

    BOOL is_dir_list;
    BOOL only_check_errors = FALSE;
    const char *filename;
    const char *ignore = "";
    BOOL is_test262_harness = FALSE;
    BOOL is_module = FALSE;

-#if !defined(_WIN32)


I guess you removed this because we are not building the binary on windows yet, right?

That's right. run-test262 didn't (and doesn't) build on win32 anyway so no point in the ifdef guard.

Turns out it did build with mingw because mingw apparently includes pthreads?

I changed it to #if !defined(__MINGW32__), I think that also covers mingw-w64.

saghul · 2024-09-29T10:41:52Z

.github/workflows/ci.yml

@@ -160,38 +160,6 @@ jobs:
        run: |
          time make test262

-  linux-gcc48:


Can we not save this one? If you are worried about the CMake checks, we could remove 262 from all and have it compiled when we run the test262* targets, WDYT?

Added a version check to CMakeLists.txt, hope that's good enough.

chqrlie · 2024-09-30T07:38:03Z

This commit introduces a couple of changes in order to make run-test262 go brr and execute tests in parallel:

Remove CONFIG_AGENT build option. The disabled version of the build was already broken and no one noticed, Remove the define altogether.

No pb with this.

Remove the -C switch. Hard to support in multi-threaded mode. I may bring it back some day because it is useful.

-C is used for the continuous integration to avoid verbose output there. Output is >400 lines for the linux test of Release version and >1400 for the asan and other slow versions. How about producing output every second or 1000 tests, whatever comes first?

Remove the -r switch. Also hard to support and I never look at test262_report.txt anyway so on the chopping block it goes.

The report file is useful for 2 purposes:

determine what test caused a crash without running the whole suite again in the debugger.
keep track of long test timings behind the scene, ie: without specifying a threshold with -T to avoid verbose output. As modified, -T is needed to get these measurements.

I understand the output to the report file is non deterministic, which might have prompted your decision to remove the option, but it could have deserved a separate PR.

Judicious use of thread-local storage so I don't have to thread through state everywhere and embiggen the diff even more.

I am not sure passing tls around would be such a problem. I guess async_done would still be a problem.

This is what Real Programmers(TM) do: stay up coding way past midnight just so the test suite finishes in one minute instead of four.

Kudos to you for this.

bnoordhuis · 2024-09-30T16:31:49Z

How about producing output every second or 1000 tests, whatever comes first?

No disagreement. The reason for removal is that the output becomes inaccurate with multiple threads: the progress thread no longer knows when exactly a test failed or succeeded, only that it did.

I understand the output to the report file is non deterministic, which might have prompted your decision to remove the option

It's not that but that it's sprinkled piecemeal across run-test262.c in an undisciplined fashion. I kind of had it working by wrapping every fprintf in a critical section but I hated the looks of it and opted to remove it altogether.

If you feel really strongly, I could bring it back, but I never use it myself and I won't miss it; -T is all I need.

bnoordhuis · 2024-09-30T16:35:10Z

Oh, one more:

I am not sure passing tls around would be such a problem.

It's inconvenient for the agent-related code. I'd have turned it into runtime or context opaque data but it already uses that for something else (and also whether it's set or not.)

Maybe not insurmountable but this felt easier / less prone to subtle bugs.

bnoordhuis added 2 commits September 29, 2024 00:55

squash! remove tcc and gcc48 buildbots

aa3386f

squash! protect stats collector with mutex

aaf71bb

saghul reviewed Sep 29, 2024

View reviewed changes

squash! bring back tcc buildbot

da40046

saghul reviewed Sep 29, 2024

View reviewed changes

bnoordhuis added 2 commits September 29, 2024 18:26

squash! bring back gcc48 buildbot

02fdb34

squash! fix mingw build

1d0d4d4

saghul approved these changes Sep 29, 2024

View reviewed changes

bnoordhuis merged commit e156452 into quickjs-ng:master Sep 30, 2024
52 checks passed

bnoordhuis deleted the fix547 branch September 30, 2024 16:35

bnoordhuis mentioned this pull request Oct 3, 2024

ThreadSanitizer support #557

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run test262 tests in parallel #564

Run test262 tests in parallel #564

bnoordhuis commented Sep 28, 2024

bnoordhuis commented Sep 28, 2024

bnoordhuis commented Sep 28, 2024

saghul left a comment

saghul Sep 29, 2024

bnoordhuis Sep 29, 2024

saghul Sep 29, 2024

bnoordhuis Sep 29, 2024

saghul Sep 29, 2024

bnoordhuis Sep 29, 2024

bnoordhuis Sep 29, 2024

saghul Sep 29, 2024

bnoordhuis Sep 29, 2024

chqrlie commented Sep 30, 2024

bnoordhuis commented Sep 30, 2024

bnoordhuis commented Sep 30, 2024

Run test262 tests in parallel #564

Run test262 tests in parallel #564

Conversation

bnoordhuis commented Sep 28, 2024

bnoordhuis commented Sep 28, 2024

bnoordhuis commented Sep 28, 2024

saghul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chqrlie commented Sep 30, 2024

bnoordhuis commented Sep 30, 2024

bnoordhuis commented Sep 30, 2024