| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
… new configure flag
|
The failure on WASI of test_trashcan_16602 is suspect because this PR changes the stack frame size of _PyEval_EvalFrameDefault. |
Sorry, something went wrong.
|
Benchmark results are in:
Talked it over with @brandtbucher. We found:
|
Sorry, something went wrong.
|
I've fixed the two issues we discovered and am firing off a new benchmark run, only for Win64, without pystats. Since I didn't move the point where the branch forks off main, I'm not sure if this will produce the right results, but it's the thing I'm most curious about. Honestly, the CALL_STAT_INC fix should probably be merged separately. |
Sorry, something went wrong.
|
Here are the Windows benchmark results. It is now only 1% slower than base -- but still, surprisingly, slower. According to the time plot, both richards and richards_super are 9% slower. What's up with that??? This occurs only on Windows; it was present on the last benchmark too (before I disabled the no-optimization pragma). |
Sorry, something went wrong.
|
Those Windows results are puzzling, but we know there is just generally more noise in the Windows results. Maybe this is a fool's errand, but we could try running them again to get a sense of the range of noise in the results? |
Sorry, something went wrong.
Hm, both runs showed similar results (in particular, both Richards being much slower). Mark's hunch is that by making the function larger, we disabled some compiler optimization that was actually a pessimization. While I'm still curious if there's a smoking gun in Richards, I am leaning towards not merging this PR. |
Sorry, something went wrong.
Yes, a bit frustratingly, it's not a clear win. |
Sorry, something went wrong.
|
I personally still think that it makes sense to make the tier two interpreter a build-time option (like the JIT) instead of a run-time option. Even if the perf impact is negligible, realistically the only people who are using tier two are building their own interpreter anyways, and it simplifies a lot of tricky edge-cases when testing tier two (for example, with subprocesses not picking up the environment variable or command-line flag). I'm not sure that we have a good argument for shipping all of tier two when pretty much nobody will be using it. |
Sorry, something went wrong.
Agreed. Then maybe I can put more stuff inside #ifndef _Py_NOTIER2, e.g. the contents of all the optimizer* files. |
Sorry, something went wrong.
|
If we were to go ahead with that idea, the configure option is misnamed -- it should probably be something like --enable-slow-tier2-interpreter, the CPP variable should be named _Py_TIER2_INTERPRETER and have the opposite sense. And we might have a separate CPP variable _Py_TIER2 that gets set by that and by the JIT flag. I'm not sure if I care to do all that work, honestly. |
Sorry, something went wrong.
|
Latest Windows benchmark: 0% faster. Richards is no longer special. (What changed: this no longer updates the bitmask/counter for POP_JUMP_IF* opcodes.) |
Sorry, something went wrong.
|
I think I'd like to rethink this after gh-116970 (moving the Tier 2 interpreter out into its own function). I'd like to almost completely skip the optimizer*.c files in that case, with the possible exception of PyUnstable_Replace_Executor, PyUnstable_SetOptimizer, PyUnstable_GetOptimizer, and PyUnstable_GetExecutor, which should allow 3rd party JITs to use this mechanism (although those aren't very useful unless we also keep something to trigger optimization that doesn't use PEP 523). |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Because this is just a quick test, I used a hack to define the crucial env var, _Py_NOTIER2, by hard-coding it in Makefile.pre.in. If this shows enough of an improvement for Tier 1 that we'd like to have this option, that hack should be replaced with a new configure option. (Adding configure options looks like black magic to me, and IIRC requires using a Docker image to build, so I'm putting that off until after we've seen benchmark results.)
Because of that hack, this fails all the JIT tests. I'll just be ignoring that.
There's also a weird test failure in the WASI build, which I'll also ignore unless we decide to move ahead with this.