GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-91054: Add code object watchers API by itamaro · Pull Request #99859 · python/cpython

Conversation

@itamaro

@itamaro itamaro commented Nov 28, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This PR implements gh-91054, allowing extensions to set a callback function to be notified on creation and destruction of code objects

pyperformance results are 1.00x slower (~neutral) using AWS bare metal machine (full comparison gist)

Co-authored-by: @Ye11ow-Flash

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
itamaro and others added 3 commits November 28, 2022 10:00
@itamaro @Ye11ow-Flash
pythongh-91054: Add API to allow extensions to set callback function …
2475ce4
…on creation and destruction of PyCodeObject

Co-authored-by: Ye11ow-Flash <[email protected]>
@itamaro @Ye11ow-Flash
Add tests for code object watcher
21db76b
Co-authored-by: Ye11ow-Flash <[email protected]>
@itamaro
Add docs and news for code watchers
7b87796
@itamaro
itamaro requested a review from markshannon as a code owner November 28, 2022 23:10
@bedevere-bot bedevere-bot mentioned this pull request Nov 28, 2022
Add API to allow extensions to set callback function on creation and destruction of PyCodeObject #91054
Closed
@bedevere-bot bedevere-bot added the awaiting review label Nov 28, 2022
itamaro added 2 commits November 28, 2022 20:29
@itamaro
Merge branch 'main' into codeobject-watchers
1d08953
@itamaro
Add Jaineel Shah (@Ye11ow-Flash) to ACKS list
ea85639
@carljm

carljm commented Nov 30, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

@itamaro I talked this over briefly with @markshannon and it sounds like we should make the same optimization here as @mpage did in 812dd5f for func watchers, to make the check for active watchers a bit more efficient and reduce the overhead on code object creation.

itamaro reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

markshannon
markshannon reviewed Nov 30, 2022
View reviewed changes
Comment thread Objects/codeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

itamaro added 2 commits November 30, 2022 11:39
@itamaro
Merge branch 'main' into codeobject-watchers
a45b44b
@itamaro
Add a bit vector to optimize watcher dispatch
c1892be
A bit is set in the bit vector iff there is a watcher set at the
corresponding offset in the watcher array. Only notify watchers
if at least one bit is set.
@markshannon

markshannon commented Dec 2, 2022

Copy link
Copy Markdown
Member

FYI, I expect to add a PY_CODE_EVENT_INSTRUMENTED event as part of PEP 669, when a code object gets instrumented by monitoring. I would expect tools to treat this new event the same as PyFunction_EVENT_MODIFY_...

carljm reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

markshannon
markshannon reviewed Dec 2, 2022
View reviewed changes
Comment thread Objects/codeobject.c
notify_code_watchers(PyCodeEvent event, PyCodeObject *co)
{
PyInterpreterState *interp = _PyInterpreterState_GET();
if (interp->active_code_watchers) {

@markshannon markshannon Dec 2, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with this as is, but you could speed this up with an invariant.
(active_code_watchers & (1 << i)) implies interp->code_watchers[i] != NULL

Then the loop can be something like this:

uint8_t watchers = interp->active_code_watchers;
while (watchers) {
    int watcher = left_most_bit(watchers);
    watchers &= ~(1 << watcher);
    if (interp->code_watchers[watcher](...) {
          PyErr_WriteUnraisable(...);
    }
}

This probably applies to the other watchers as well, so you might want to do this in another PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@carljm carljm Dec 2, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this optimization applies to func watchers, but is already effectively applied for dict and type watchers; in those cases we already never look up the watcher unless the bit is set.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@itamaro itamaro Dec 2, 2022

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some more watchers-wide cleanup I wanted to do, so will add this optimization to the next PR, thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@itamaro itamaro Dec 5, 2022

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleanup and polish PR (including this optimization) is up at GH-99998

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
markshannon
markshannon approved these changes Dec 2, 2022
View reviewed changes

@markshannon markshannon left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Does this complete the set of watchers?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Dec 2, 2022
@carljm

carljm commented Dec 2, 2022

Copy link
Copy Markdown
Member

Looks good. Does this complete the set of watchers?

Yep! (At least the ones we had in mind.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon
markshannon merged commit 3c137dc into python:main Dec 2, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Dec 2, 2022
@bedevere-bot

bedevere-bot commented Dec 2, 2022

Copy link
Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot wasm32-emscripten node (pthreads) 3.x has failed when building commit 3c137dc.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1050/builds/953) and take a look at the build logs.
  4. Check if the failure is related to this commit (3c137dc) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1050/builds/953

Failed tests:

  • test_capi

Summary of the results of the build (if available):

== Tests result: FAILURE ==

338 tests OK.

10 slowest tests:

  • test_tokenize: 58.6 sec
  • test_unparse: 51.9 sec
  • test_lib2to3: 50.1 sec
  • test_io: 27.6 sec
  • test_unicodedata: 21.8 sec
  • test_pickle: 17.0 sec
  • test_statistics: 16.9 sec
  • test_weakref: 16.8 sec
  • test_zipfile: 15.6 sec
  • test_decimal: 15.5 sec

1 test failed:
test_capi

94 tests skipped:
test__xxsubinterpreters test_asyncgen test_asyncio
test_check_c_globals test_clinic test_cmd_line
test_concurrent_futures test_contextlib_async test_ctypes
test_curses test_dbm_gnu test_dbm_ndbm test_devpoll test_doctest
test_docxmlrpc test_dtrace test_embed test_epoll test_faulthandler
test_fcntl test_file_eintr test_fork1 test_ftplib test_gdb
test_grp test_httplib test_httpservers test_idle test_imaplib
test_interpreters test_ioctl test_kqueue test_launcher test_lzma
test_mmap test_msilib test_multiprocessing_fork
test_multiprocessing_forkserver test_multiprocessing_main_handling
test_multiprocessing_spawn test_nis test_openpty test_ossaudiodev
test_pdb test_peg_generator test_perf_profiler test_poll
test_poplib test_pty test_pwd test_readline test_regrtest
test_repl test_resource test_select test_selectors test_smtplib
test_smtpnet test_socket test_socketserver test_spwd test_ssl
test_stable_abi_ctypes test_startfile test_subprocess
test_sys_settrace test_syslog test_tcl test_telnetlib test_tix
test_tkinter test_tools test_ttk test_ttk_textonly test_turtle
test_urllib2 test_urllib2_localnet test_urllib2net test_urllibnet
test_venv test_wait3 test_wait4 test_webbrowser test_winconsoleio
test_winreg test_winsound test_wmi test_wsgiref test_xmlrpc
test_xmlrpc_net test_xxlimited test_zipfile64
test_zipimport_support test_zoneinfo
0:20:50 load avg: 6.85
0:20:50 load avg: 6.85 Re-running failed tests is not supported with --python host runner option.

Total duration: 20 min 50 sec

Click to see traceback logs
remote: Enumerating objects: 30, done.        
remote: Counting objects:   3% (1/28)        
remote: Counting objects:   7% (2/28)        
remote: Counting objects:  10% (3/28)        
remote: Counting objects:  14% (4/28)        
remote: Counting objects:  17% (5/28)        
remote: Counting objects:  21% (6/28)        
remote: Counting objects:  25% (7/28)        
remote: Counting objects:  28% (8/28)        
remote: Counting objects:  32% (9/28)        
remote: Counting objects:  35% (10/28)        
remote: Counting objects:  39% (11/28)        
remote: Counting objects:  42% (12/28)        
remote: Counting objects:  46% (13/28)        
remote: Counting objects:  50% (14/28)        
remote: Counting objects:  53% (15/28)        
remote: Counting objects:  57% (16/28)        
remote: Counting objects:  60% (17/28)        
remote: Counting objects:  64% (18/28)        
remote: Counting objects:  67% (19/28)        
remote: Counting objects:  71% (20/28)        
remote: Counting objects:  75% (21/28)        
remote: Counting objects:  78% (22/28)        
remote: Counting objects:  82% (23/28)        
remote: Counting objects:  85% (24/28)        
remote: Counting objects:  89% (25/28)        
remote: Counting objects:  92% (26/28)        
remote: Counting objects:  96% (27/28)        
remote: Counting objects: 100% (28/28)        
remote: Counting objects: 100% (28/28), done.        
remote: Compressing objects:   7% (1/13)        
remote: Compressing objects:  15% (2/13)        
remote: Compressing objects:  23% (3/13)        
remote: Compressing objects:  30% (4/13)        
remote: Compressing objects:  38% (5/13)        
remote: Compressing objects:  46% (6/13)        
remote: Compressing objects:  53% (7/13)        
remote: Compressing objects:  61% (8/13)        
remote: Compressing objects:  69% (9/13)        
remote: Compressing objects:  76% (10/13)        
remote: Compressing objects:  84% (11/13)        
remote: Compressing objects:  92% (12/13)        
remote: Compressing objects: 100% (13/13)        
remote: Compressing objects: 100% (13/13), done.        
remote: Total 30 (delta 15), reused 15 (delta 15), pack-reused 2        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to '3c137dc613c860f605d3520d7fd722cd8ed79da6'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 3c137dc613 GH-91054: Add code object watchers API (GH-99859)
Switched to and reset branch 'main'

../../Objects/obmalloc.c:776:1: warning: always_inline function might not be inlinable [-Wattributes]
  776 | arena_map_get(pymem_block *p, int create)
      | ^~~~~~~~~~~~~

configure: ../../configure --prefix $(PWD)/target/host --with-pydebug --without-pydebug --with-emscripten-target=node --disable-wasm-dynamic-linking --enable-wasm-pthreads --build=x86_64-pc-linux-gnu --host=wasm32-unknown-emscripten --with-build-python=../build/python
configure: WARNING: using cross tools not prefixed with host triplet
mcc: error: no input files

make: make -j2 all
../../Python/initconfig.c:2347:27: warning: format specifies type 'wint_t' (aka 'int') but the argument has type 'wint_t' (aka 'unsigned int') [-Wformat]
    printf(usage_envvars, (wint_t)DELIM, (wint_t)DELIM, PYTHONHOMEHELP);
           ~~~~~~~~~~~~~  ^~~~~~~~~~~~~
../../Python/initconfig.c:138:18: note: format string is defined here
"PYTHONPATH   : '%lc'-separated list of directories prefixed to the\n"
                 ^~~
                 %c
../../Python/initconfig.c:2347:42: warning: format specifies type 'wint_t' (aka 'int') but the argument has type 'wint_t' (aka 'unsigned int') [-Wformat]
    printf(usage_envvars, (wint_t)DELIM, (wint_t)DELIM, PYTHONHOMEHELP);
           ~~~~~~~~~~~~~                 ^~~~~~~~~~~~~
../../Python/initconfig.c:141:58: note: format string is defined here
"PYTHONHOME   : alternate <prefix> directory (or <prefix>%lc<exec_prefix>).\n"
                                                         ^~~
                                                         %c
2 warnings generated.
../../Python/pytime.c:297:10: warning: implicit conversion from 'long long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Wimplicit-const-int-float-conversion]
    if (!_Py_InIntegralTypeRange(time_t, intpart)) {
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Include/internal/pycore_pymath.h:72:45: note: expanded from macro '_Py_InIntegralTypeRange'
    (_Py_IntegralTypeMin(type) <= v && v <= _Py_IntegralTypeMax(type))
                                         ~~ ^~~~~~~~~~~~~~~~~~~~~~~~~
../../Include/internal/pycore_pymath.h:61:88: note: expanded from macro '_Py_IntegralTypeMax'
    (_Py_IS_TYPE_SIGNED(type) ? (((((type)1 << (sizeof(type)*CHAR_BIT - 2)) - 1) << 1) + 1) : ~(type)0)
                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
../../Python/pytime.c:352:14: warning: implicit conversion from 'long long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Wimplicit-const-int-float-conversion]
        if (!_Py_InIntegralTypeRange(time_t, intpart)) {
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Include/internal/pycore_pymath.h:72:45: note: expanded from macro '_Py_InIntegralTypeRange'
    (_Py_IntegralTypeMin(type) <= v && v <= _Py_IntegralTypeMax(type))
                                         ~~ ^~~~~~~~~~~~~~~~~~~~~~~~~
../../Include/internal/pycore_pymath.h:61:88: note: expanded from macro '_Py_IntegralTypeMax'
    (_Py_IS_TYPE_SIGNED(type) ? (((((type)1 << (sizeof(type)*CHAR_BIT - 2)) - 1) << 1) + 1) : ~(type)0)
                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
../../Python/pytime.c:518:10: warning: implicit conversion from 'long long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Wimplicit-const-int-float-conversion]
    if (!_Py_InIntegralTypeRange(_PyTime_t, d)) {
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Include/internal/pycore_pymath.h:72:45: note: expanded from macro '_Py_InIntegralTypeRange'
    (_Py_IntegralTypeMin(type) <= v && v <= _Py_IntegralTypeMax(type))
                                         ~~ ^~~~~~~~~~~~~~~~~~~~~~~~~
../../Include/internal/pycore_pymath.h:61:88: note: expanded from macro '_Py_IntegralTypeMax'
    (_Py_IS_TYPE_SIGNED(type) ? (((((type)1 << (sizeof(type)*CHAR_BIT - 2)) - 1) << 1) + 1) : ~(type)0)
                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
3 warnings generated.
../../Modules/expat/xmlparse.c:3116:9: warning: code will never be executed [-Wunreachable-code]
        parser->m_characterDataHandler(parser->m_handlerArg, parser->m_dataBuf,
        ^~~~~~
../../Modules/expat/xmlparse.c:3115:16: note: silence by adding parentheses to mark code as explicitly dead
      else if (0 && parser->m_characterDataHandler)
               ^
               /* DISABLES CODE */ ( )
../../Modules/expat/xmlparse.c:4059:9: warning: code will never be executed [-Wunreachable-code]
        parser->m_characterDataHandler(parser->m_handlerArg, parser->m_dataBuf,
        ^~~~~~
../../Modules/expat/xmlparse.c:4058:16: note: silence by adding parentheses to mark code as explicitly dead
      else if (0 && parser->m_characterDataHandler)
               ^
               /* DISABLES CODE */ ( )
../../Modules/expat/xmlparse.c:7703:11: warning: format specifies type 'int' but the argument has type 'ptrdiff_t' (aka 'long') [-Wformat]
          bytesMore, (account == XML_ACCOUNT_DIRECT) ? "DIR" : "EXP",
          ^~~~~~~~~
3 warnings generated.
../../Modules/socketmodule.c:4031:33: warning: comparison of integers of different signs: 'unsigned long' and 'long' [-Wsign-compare]
         cmsgh != NULL; cmsgh = CMSG_NXTHDR(&msg, cmsgh)) {
                                ^~~~~~~~~~~~~~~~~~~~~~~~
/opt/buildbot/.emscripten_cache/sysroot/include/sys/socket.h:356:44: note: expanded from macro 'CMSG_NXTHDR'
        __CMSG_LEN(cmsg) + sizeof(struct cmsghdr) >= __MHDR_END(mhdr) - (unsigned char *)(cmsg) \
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Modules/socketmodule.c:4084:33: warning: comparison of integers of different signs: 'unsigned long' and 'long' [-Wsign-compare]
         cmsgh != NULL; cmsgh = CMSG_NXTHDR(&msg, cmsgh)) {
                                ^~~~~~~~~~~~~~~~~~~~~~~~
/opt/buildbot/.emscripten_cache/sysroot/include/sys/socket.h:356:44: note: expanded from macro 'CMSG_NXTHDR'
        __CMSG_LEN(cmsg) + sizeof(struct cmsghdr) >= __MHDR_END(mhdr) - (unsigned char *)(cmsg) \
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Modules/socketmodule.c:4708:54: warning: comparison of integers of different signs: 'unsigned long' and 'long' [-Wsign-compare]
            cmsgh = (i == 0) ? CMSG_FIRSTHDR(&msg) : CMSG_NXTHDR(&msg, cmsgh);
                                                     ^~~~~~~~~~~~~~~~~~~~~~~~
/opt/buildbot/.emscripten_cache/sysroot/include/sys/socket.h:356:44: note: expanded from macro 'CMSG_NXTHDR'
        __CMSG_LEN(cmsg) + sizeof(struct cmsghdr) >= __MHDR_END(mhdr) - (unsigned char *)(cmsg) \
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 warnings generated.
../../Modules/_sqlite/connection.c:2198:19: warning: result of comparison of constant 9223372036854775807 with expression of type 'Py_ssize_t' (aka 'long') is always false [-Wtautological-constant-out-of-range-compare]
    if (data->len > 9223372036854775807) {  // (1 << 63) - 1
        ~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~
1 warning generated.
mcc: warning: USE_PTHREADS + ALLOW_MEMORY_GROWTH may run non-wasm code slowly, see https://github.com/WebAssembly/design/issues/1271 [-Wpthreads-mem-growth]

make: *** [Makefile:1860: buildbottest] Error 2

Cannot open file '/opt/buildbot/bcannon-wasm/3.x.bcannon-wasm.emscripten-node-pthreads/build/build/build_oot/host/test-results.xml' for upload
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brettcannon

brettcannon commented Dec 2, 2022

Copy link
Copy Markdown
Member

FYI that buildbot failure doesn't seem to be transient. Are there new tests which are recursive and assuming a certain stack depth? Looking at the test failure it seems the call stack is being blown out in the emscripten build (line 515 of https://buildbot.python.org/all/#/builders/1050/builds/953/steps/10/logs/stdio).

itamaro reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal

pablogsal commented Dec 3, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There are many buildbots failing to build this PR. Check https://buildbot.python.org/all/#/release_status for reference

As per our buildbot policy, this PR will be reverted if the buildbot fleet is not fixed in the next 24h. @markshannon @itamaro can you please take a look?

I have opened #99976. I will close it if the issue is fixed before the deadline.

itamaro reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal pablogsal mentioned this pull request Dec 3, 2022
gh-91054: Revert "gh-91054: Add code object watchers API" #99976
Closed
pablogsal added a commit that referenced this pull request Dec 3, 2022
@pablogsal
Revert "GH-91054: Add code object watchers API (GH-99859)"
9cf2109
This reverts commit 3c137dc.
itamaro added a commit to itamaro/cpython that referenced this pull request Dec 3, 2022
@itamaro
pythonGH-91054: Reset static events counts in code watchers tests
5a53c06
pythonGH-99859 introduced new buildbot failures, as reported [here](python#91054 (comment)).

I was able to reproduce the failures with:

```
./python.exe -m test -v test_capi.test_watchers -m "*TestCodeObjectWatchers*" -R 3:3
```

The root cause appears to be to static events counters used in the tests,
when running the tests with repetitions (using the same interpreter state),
the counts from the first test run affected the next runs.

This fixes it by resetting the counts when adding and clearing test watchers.
@itamaro itamaro mentioned this pull request Dec 3, 2022
GH-91054: Reset static events counts in code watchers tests #99978
Merged
@itamaro

itamaro commented Dec 3, 2022

Copy link
Copy Markdown
Contributor Author

Thanks @pablogsal! I have opened GH-99978 to fix the issue. My local repro passes with the fix - could you get all buildbots to test this PR to confirm?

@brettcannon I couldn't find anything in this PR that could trigger deep recursion. I wonder if it's the same issue caught by the other buildbots, but manifesting in a different way on the emscripten buildbot?
do you have documentation on how to set up a local emscripten build in case the fix in GH-99978 is not sufficient?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal

pablogsal commented Dec 3, 2022

Copy link
Copy Markdown
Member

Thanks @pablogsal! I have opened GH-99978 to fix the issue. My local repro passes with the fix - could you get all buildbots to test this PR to confirm?

Yup, checking!

itamaro reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brettcannon

brettcannon commented Dec 5, 2022

Copy link
Copy Markdown
Member

@itamaro https://github.com/python/cpython/tree/main/Tools/wasm covers how to do anything WebAssembly-related.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@itamaro

itamaro commented Dec 7, 2022

Copy link
Copy Markdown
Contributor Author

thanks @brettcannon, sorry about the delay, I spent the last two days trying to repro and debug this 😬

TLDR: I think this PR may have pushed something over some threshold that caused the junit xml generation to start hitting stack depth limits. (but is not the root cause of the failures)

the steps in the docs failed (basically running ./Tools/wasm/wasm_build.py in the docker container couldn't find the emsdk or something like that), and also didn't apply to the "node (pthreads)" target, which is the one failing.

I was able to repro the failure using some mix of the instructions and reverse engineering the buildbot itself.

TLDR build and test steps, inside the docker container:

mkdir -p build_oot/build build_oot/host
cd build_oot/build
../../configure && make -j
cd ../host
CONFIG_SITE=../../Tools/wasm/config.site-wasm32-emscripten emconfigure ../../configure --prefix '$(PWD)/target/host' --with-pydebug --without-pydebug --with-emscripten-target=node --disable-wasm-dynamic-linking --enable-wasm-pthreads --build=x86_64-pc-linux-gnu --host=wasm32-unknown-emscripten --with-build-python=$(pwd)/../build/python --build=$(../../config.guess) && emmake make -j2 all
make buildbottest 'TESTOPTS=-j2 --junit-xml test-results.xml -j2 ${BUILDBOT_TESTOPTS}' TESTPYTHONOPTS= TESTTIMEOUT=900

this is more or less what the buildbot does, and it fails (not always, but almost always).

this also runs the full test suite, but I was able to narrow it down to running just test_capi and still fail:

root@18f2ffc1b119:/python-wasm/cpython/build_oot/host# _PYTHON_HOSTRUNNER='/emsdk/node/14.18.2_64bit/bin/node --experimental-wasm-bigint --experimental-wasm-threads --experimental-wasm-bulk-memory' _PYTHON_PROJECT_BASE=/python-wasm/cpython/build_oot/host _PYTHON_HOST_PLATFORM=emscripten-wasm32 PYTHONPATH=/python-wasm/cpython/build_oot/host/build/lib.emscripten-wasm32-3.12:../../Lib _PYTHON_SYSCONFIGDATA_NAME=_sysconfigdata__emscripten_wasm32-emscripten /python-wasm/cpython/build_oot/host/../build/python  ../../Tools/scripts/run_tests.py --junit-xml test-results.xml test_capi

what makes me think this PR is not the root cause of the regression is that it consistently passes when running just the watchers tests:

root@18f2ffc1b119:/python-wasm/cpython/build_oot/host# _PYTHON_HOSTRUNNER='/emsdk/node/14.18.2_64bit/bin/node --experimental-wasm-bigint --experimental-wasm-threads --experimental-wasm-bulk-memory' _PYTHON_PROJECT_BASE=/python-wasm/cpython/build_oot/host _PYTHON_HOST_PLATFORM=emscripten-wasm32 PYTHONPATH=/python-wasm/cpython/build_oot/host/build/lib.emscripten-wasm32-3.12:../../Lib _PYTHON_SYSCONFIGDATA_NAME=_sysconfigdata__emscripten_wasm32-emscripten /python-wasm/cpython/build_oot/host/../build/python  ../../Tools/scripts/run_tests.py --junit-xml test-results.xml test_capi.test_watchers

and what makes me suspect the root cause is in junit xml generation is that dropping that flag causes test_capi to pass consistently:

root@18f2ffc1b119:/python-wasm/cpython/build_oot/host# _PYTHON_HOSTRUNNER='/emsdk/node/14.18.2_64bit/bin/node --experimental-wasm-bigint --experimental-wasm-threads --experimental-wasm-bulk-memory' _PYTHON_PROJECT_BASE=/python-wasm/cpython/build_oot/host _PYTHON_HOST_PLATFORM=emscripten-wasm32 PYTHONPATH=/python-wasm/cpython/build_oot/host/build/lib.emscripten-wasm32-3.12:../../Lib _PYTHON_SYSCONFIGDATA_NAME=_sysconfigdata__emscripten_wasm32-emscripten /python-wasm/cpython/build_oot/host/../build/python  ../../Tools/scripts/run_tests.py test_capi

I don't know enough about wasm and junit xml to dig deeper for a fix, but I hope this investigation can provide someone who's more familiar with wasm and junit xml with enough information to take it from here!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brettcannon

brettcannon commented Dec 8, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Thanks for the research! It looks like yesterday some change was made that brought the builder back to being stable. But if it happens again I will definitely come back to your notes to see if we may need to tweak some code in how we're generating that junit XML output.

itamaro reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

facebook-github-bot pushed a commit to facebookincubator/MetaPython that referenced this pull request Jul 6, 2023
Carl Meyer @facebook-github-bot
backport 3.12 code watchers
7cc60de
Summary: Backport of python/cpython#99859 (plus some more recent changes.)

Reviewed By: alexmalyshev

Differential Revision: D47201113

fbshipit-source-id: 9390ca289c043bc56ebfdd2ade63305347190a99
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm left review comments
@markshannon markshannon markshannon approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@itamaro @carljm @markshannon @bedevere-bot @brettcannon @pablogsal
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.