Test emterpreter sync support #3129
Comments
|
Tested on BananaBread as well, seems to work ok. |
|
Closer testing on boon shows a rendering glitch in the emterpreter, the gunshots hitting walls look wrong. Unrelated to async, though. Perhaps I should fuzz the emterpreter. |
|
DOSBox seems to work correctly with emterpreter, but is unusably slow, even with an old game (Duke Nukem 1). With some of my modifications disabled and sync enabled, I was able to interactively execute several commands at the DOS prompt, though inputting characters was very slow. Interactive use of the DOS prompt was impossible before because the way that code is structured needed sync. I expect the blacklist could be used to improve performance while taking advantage of sync for some specific cases that were preventing significant numbers of games from running. Making all cases work wouldn't be good. The CPU interpreter should be sped up by not using emterpreter, but it can call itself recursively. This would involve listing a huge number of functions though. Can I use a file for the blacklist or use a whitelist instead? |
|
Yeah, I would expect a CPU emulator to be too slow without a blacklist. Good to hear though that the async stuff seems to work (slowly). Recursion by itself isn't a problem, normal asm.js stack frames can mix with emterpreted ones without problems - except for when doing sleep and other sync stuff. Is it possible for the core CPU interpreter to be on the stack when doing a sleep? In that case, a blacklist won't help, I'm afraid. Otherwise, if it's just called recursively, then blacklisting the one CPU interpreter function should give you most of the speed. You can already use a file for the blacklist, I added a test now to be sure. |
|
If I use This is with latest incoming. I didn't even know Linux had a limit. This is with 4587 functions. |
|
Ah, right, we have code to expand those out early, and we do send all SETTINGS stuff to all parts of the compiler. Fixed on incoming. |
|
(Regarding the boon bug I mentioned before, it wasn't emterpreter-sync or even emterpreter-related, but it only triggered in the emterpreter due to different optimizations being run. Anyhow, fixed on incoming, boon looks correct now.) |
|
Thanks for your quick fixes, you're amazing! I've managed to use a huge blacklist, which runs correctly but slowly as long as the CPU interpreter is not in the blacklist. With the CPU interpreter in the blacklist, it fails. I don't see how emscripten_sleep() could be called from the CPU interpreter with the way I'm trying to use it in my tests. I'm not sure how to examine what's going on and trial and error is getting frustrating so I'll give up on this for now. |
|
Yeah, if the CPU interpreter is on the stack when a sleep is called, then it should not be in the blacklist. So this limits speed here significantly. I wonder if you can tell before calling the CPU interpreter whether it will sleep? If so there could be 2 versions, one blacklisted and one not. But maybe this is all too complicated. Anyhow, thanks for trying this out. |
|
Is a problem only supposed to occur if there is a blacklisted function on the stack when sleep occurs? (ie. if you have Is it perfectly okay to call a blacklisted function before or after a sleep as long as the function returns before the next sleep? (ie. Suppose If my understanding so far is correct, I should be able to take advantage of sync to increase the amount of stuff that works in DOSBox. The paths which might call sleep via the CPU interpreter are CPU exceptions, and those should generally be handled quickly, so sleep can be skipped there. I have a problem. In the same .js file and the same function, sleep works if the function is called via one call stack and doesn't return if the function is called via another call stack. This happens even without blacklisting. The call stack consists of a multiple calls to the emterpreter function and a few small functions which push arguments onto the stack and then call the emterpreter. If I comment out the sleep, the script is non-responsive, but the output I get when the non-responsive script dialog occurs shows that it works properly otherwise. Edit: By moving the emscripten_sleep() back in the call chain I found a more precise location where something goes wrong. It works fine here, before the call to Edit: The problem seems to happen when calling a virtual function via a pointer: asynctest.h: asynctest1.cpp asynctest2.cpp Build with |
|
Thanks, the issue was not virtual functions specifically but having functions returning a value, on the stack, when saving it. I had only tested void functions... ;) Fixed on incoming. |
|
I'm pretty sure I tried returning values, and it seemed virtual needed to be there to trigger the problem. Anyways, with your change, DOSBox works better. I can get to the DOS prompt and watch the blinking cursor, but if I press a key I keep getting continuous fake input. Experimentation shows that if I put a sleep at the start of Edit: device_CON inherits from DOS_Device, which inherits from DOS_File. In the backtrace, I see a call to DOS_Device::Read(), which then calls device_CON::Read(). Initially, both are passed different this pointers. After the sleep, both get the this pointer of DOS_Device::Read(). |
|
I compiled the following with #include <emscripten.h>
#include <stdio.h>
class DOS_Device {
int devnum;
public:
DOS_Device() { devnum = 0; }
virtual bool Read(unsigned char * data,unsigned short * size);
};
DOS_Device *Devices[10];
bool __attribute__((noinline)) DOS_Device::Read(unsigned char * data,unsigned short * size) {
printf("DOS_Device::Read (this = %i)\n", (int)this);
return Devices[devnum]->Read(data,size);
}
class device_CON : public DOS_Device {
public:
bool Read(unsigned char * data,unsigned short * size);
};
bool device_CON::Read(unsigned char * data,unsigned short * size) {
//emscripten_log(EM_LOG_C_STACK, "Sleep CON:Read");
printf("device_CON::Read (this = %i) Sleep--> \n", (int)this);
emscripten_sleep(1000);
printf("<--Sleep (this = %i)\n", (int)this);
return true;
}
int main(void) {
device_CON con;
Devices[0] = &con;
DOS_Device dev;
dev.Read(0,0);
} |
|
I suspect virtual was needed before, because otherwise LLVM inlines and gets rid of the call. Thanks for the new testcase, will take a look soon. |
|
Fixed, the trampolines didn't notice async state. This is a problem that does only occur with virtual methods, as otherwise when restoring the stack, trampolines are not used (we just do |
|
Thanks! The DOSBox prompt was now usable, and I could run programs. I still need to work on the blacklist to improve performance and make games playable. Edit: Here's a build with a blacklist and decent performance. I did not encounter any more sync problems. However, I can't link that exact same thing with emterpreter but without the blacklist. That is followed by a whole lot of JavaScript. While trying to narrow down what file is causing this problem, I found an object file which triggers an error by itself with -O2, in particular Edit: Compilation with |
|
I'm wondering about how to automatically create a blacklist for DOSBox. I can't use a static blacklist because some name mangling changes. My current blacklist is based on printing |
|
Ok, I added a whitelist option now, which takes precedence over the blacklist. |
|
Thanks! The whitelist was easy to use and I did not find any more problems with DOSBox running with emterpreter sync. The only remaining issue I'm aware of is that the final link leaves files in tmp directories, like: Edit: I was doing all my testing with SDL 2. When I compiled DOSBox with SDL 1, sound was broken like due to #3122. I will post a new comment if I make significant discoveries showing a problem in Emscripten. Archive.org cannot use the emterpreter sync builds because they're unable to support the memory init file. I see how emterpretify.py is designed to use that file, but I can't think of any theoretical reason why that can't be changed and a memory init file is unavoidable when using emterpreter. I realize this is not a bug and I feel reluctant to actually ask for this change. |
|
MESS blows it up: Files are at http://interbutt.com/temp/issue3129.zip |
|
Thanks, fixed (might need |
|
Thanks! It worked, and does seem to lower CPU usage for less-demanding emulations that were spending time in usleep() before. However in the more-demanding case there is a substantial speed hit so I will stick to emscripten_set_main_loop for now. |
|
Use of emterpreter sync makes Em-DOSBox SDL 1 audio much worse. The audio callbacks are delivered in bunches and DOSBox cannot handle the jitter, like in #3122 (which is now fixed). This is not a new problem; I just didn't investigate it before because I was using SDL 2. It seems SDL 1 audio depends on the This is not due to DOSBox calling Edit: Adding a similar line for queuing audio in |
|
It sounds like that problem would be fixed if edit: ah, I see this is what you said in the edit? do I understand correctly? |
|
Okay, I guess an API is a better idea than just adding that line. SDL 2 or other stuff might need it later. I wonder if the same API should be used for adding stuff to run after the main loop. In other words, if something is added, it runs after the main loop and it runs when |
|
Just to clarify: this is the SDL 1 audio fix mentioned in the edit of my previous comment. I did not submit it as a pull request now because you talked about adding "a general method to add things for |
|
Ok, implemented in fd797ce . Please let me know if that doesn't work (we don't have a test for it yet). |
|
Thanks! Your fix works. Em-DOSBox sound is now good when using SDL 1 and emterpreter sync. I guess an automated test could measure sound jitter. |
|
It seems that #include <stdio.h>
#include <emscripten.h>
int main(void) {
printf("No yield:\n");
emscripten_sleep(500);
printf("With yield:\n");
emscripten_sleep_with_yield(500);
printf("Done\n");
return 0;
}It works if both calls are I first ran into a problem trying to use |
|
The problem happens when there is an This could be fixed by adding |
|
Thanks! Fixed and added that test. |
|
Disregard stuff I wrote previously about a line after
|
|
Great! Can you please verify that you get an assertion if you don't have that method in the whitelist, and the line runs twice? When building with ASSERTIONS. |
|
I have re-built with |
|
That -12 should be saying that an async operation is happening with non-emterpreted code on the stack. Likely those two methods need to be added to the whitelist. |
|
I cannot imagine why those methods need to be added to the whitelist, because they won't be interrupted by sleep. The assertion is happening because they are being called during Since SDL 1 is mostly written in JS, I only needed to comment out that one |
|
If a function in It seems there's no code for this in |
|
Regarding the earlier comment: I see. Ok, sounds like we need to be able to mark some methods as ok to run while yielding, a YIELDLIST I guess. This is a bit why I was worried about even having a yield mode - it makes the model complicated - but hopefully this is as complicated as it gets. I'll look into this now. About the last comment: yeah, pull request to error on missing whitelist functions would be great. Testing would also be good, in |
|
Ok, the YIELDLIST option has been added, with an sdl1 audio test that adds the relevant methods added to the yield list, so that assertions do not trigger, in bc1426f |
|
I confirm that the SDL 1 sound This also seems to remove all sleep protection from functions in I was thinking that checks could use the value of Of course the test could use |
|
Good point, that would be an improvement. 1 would need to be "going to sleep or sleeping with yield", since we don't currently have a way to switch between "going to sleep" and "sleeping" (we can't tell when the stack has unwound). Otherwise that sounds great. Not sure if I'll have time for this soon, feel free to work on it if you want. |
|
It seems that after However, sleep can also happen in code that wasn't started via I wonder if it's possible to break things by calling sleep in code started elsewhere? Experiments led to the following somewhat unrelated issues. Here #include <stdio.h>
#include <emscripten.h>
void main_loop(void) {
emscripten_sleep_with_yield(1);
printf("Main loop\n");
}
int main(void) {
emscripten_set_main_loop(main_loop, 1, 1);
}Here you get: #include <emscripten.h>
int main(void) {
emscripten_sleep(1);
emscripten_exit_with_live_runtime();
}Edit: Now I see that |
|
The first testcase was a bug in the mainloop code - resuming a main loop always did requestAnimationFrame, at 60fps. Fixed on incoming. I guess this wasn't noticed without the emterpreter which relies on mainloop resuming more heavily. |
|
The second issue is indeed a little surprising. I'm not sure how to improve it, though. |
|
I got a bug report about input events like an SDL InputHandler. We call those directly from the event handler code, so that a user input => compiled InputHandler => fullscreen or another feature which needs to directly result from a user input. That should be fixed now on incoming. I did need to refactor a bunch of stuff, so I hope I didn't break anything. Test suite so far looks fine. edit: InputHandlers need to be in the yield list, to not trigger asserts. |
incoming branch now has support for async/sync code in the emterpreter. That is, when running in the emterpreter, you can write sync code like most programs do, with
sleep()and such, and it can still work in a browser. The interpreter loop saves it's state, does a setTimeout, then rebuilds and resumes the state. For more details see https://github.com/kripken/emscripten/wiki/Emterpreter#synchronous-codeThis might be useful to emulators, ccing some people: @dreamlayers, @caiiiycuk . So far I have some basic tests in the test suite that pass, and I tried SDL-Doom, which worked (even effects like wipe work, which are synchronous, and there was no need to use emscripten_set_main_loop etc.). But this probably needs a lot more testing.
The text was updated successfully, but these errors were encountered: