Make ASYNCIFY_ADD propagate (or add a setting to do so) #13150
Comments
|
Neither add nor remove propagate. The current model is that we propagate using all the info we know, and then apply the add and remove changes afterwards. I agree an option for a propagating add could make sense. It wouldn't always, though, so it can't replace the old one (for example, you may want to add X but know it will only be called from/by only some of the functions that can call it/be called by it). Perhaps there could be a "hint" list, which is like "add" but is done before propagation. Is it hard to add all the functions that you need, though? maybe we should make that easier. The |
|
I'm not sure I understand your argument against always propagating. If you know that a function is never called by one of its callers then aren't you saying that a code path is known to be dead. I that case wouldn't it be best to remove the codepath completely (using #ifdef or some other method). Culling such dead codepaths is particularly beneficial in emscripten. Leaving such a suspected-dead codepath in the codebase would also be dangerous since if it did turns out not to be dead after all then you end be calling a async function from a non-async function which would break right? In other words isn't it reasonable to say "If you don't want a function marked as async, make sure it doesn't transitively call any other async functions". |
I don't know how many I would need to add (I haven't even yet identified all of the indirect call functions that do need to be async), but it could easily be dozens or more. Listing them all wouldn't be fun, especially not if optimising/inlining changes the list. And even asyncify-verbose isn't all that useful as it only gives one reason for each function, so we can't just rely on grepping the output. In my mind this is very much a task for the computer, not humans. ;) |
|
I'm trying to asyncify a relatively small portion of a large codebase where manually adding all the caller functions would be extremely tedious and error-prone (I have 35k instrumented functions with ASYNCIFY_IGNORE_INDIRECT), and break upon any minor code refactoring anywhere in the call chain. Perhaps ASYNCIFY_IMPORTS could be extended to work for arbitrary C functions? This would be extremely helpful to us. |
|
Upon further reflection, it would actually be even more convenient if we had an |
|
And attributes won't help for all usecases. I bring in the app I'm porting as an unmodified submodule, so controlling compilation and linking external is essential. |
|
@curiousdannii Yes, we definitely will keep the external API for this regardless of adding a new method with attributes. |
|
It wouldn't be as simple as moving this code: Before this code: Would it? (Btw, it looks like if you provide an only-list then it still does all the rest of the processing as if you hadn't. That's a waste of processor time, right?) |
|
I'm not sure, but yes, I think that could reverse the logic if I'm not missing something. But again, that would need be optional, or a new flag. The design is the harder question, I think. (Yes, there might be some wasted work there. By far the most work in the pass is flattening the IR and re-optimizing it, so it's no big deal to do more propagation.) |
|
If adding an attribute is difficult, perhaps in the shorter term we could just have a call like |
|
This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant. |
|
This is still important. Help would be very welcome! |
curiousdannii
changed the title
|
I think the best path forward here is a new option. Changing the behavior of the existing one should not break users, but it could increase their code size very significantly, which worries me. I do understand the point of view that always propagating is safer and better, but I think there is also a case for the reverse: sometimes a user knows a function should be added but does not want everything they call to also be added. They could manually remove those calls in the code (as mentioned above), and that would be better, but it may not be practical. Overall, the smallest code size can be achieved by finding all the stack traces needed in practice at runtime and adding those, without the things they call, which is what the current mode allows without any source refactoring. Adding a new option should be pretty simple, I think. Perhaps |
|
ASYNCIFY_ADD_PROPAGATED? ASYNCIFY_REMOVE seems to already propagate.
It's the other way round: all callers to these functions need to be instrumented, not everything they call. Which I'm sure you know! But writing about these things can be tricky ;) |
|
Maybe there could be both an option (I don't care what it's called, but I might offer ASYNCIFY_MARK_INDIRECT) as well as an attribute that could be applied in the source code that has the same effect? |
Oh, yes, I had that reversed... 😄
Ah, I guess it does, in the sense that it prevents propagation that would have happened if it were not removed? Fair point. Maybe I'm too negative on the idea of just making add propagate. I'd feel more comfortable if we had more feedback from users of Asyncify atm. ping @caiiiycuk @gabrielcuvillier - are you perhaps still using Asyncify these days, and using |
|
On my side I generally prefer to have full and explicit control of which functions will be asyncified, but that's because I ported programs that are both large and performance sensitive (ie. a big game loop in d3wasm for example).
So... I manually explore all the various code paths that might lead to functions that require asyncify for yielding (90% of the time: UI modals or loading screens/bars inside a main loop), and mark everything in the call stack as candidate. After further analysis, I discovered it is frequently possible to refactor the code to separate what is needed for asyncify purposes, allowing to really fine tune the asyncify path to the absolute strict minimum.
In the end I am not really using ASYNCIFY_ADD or REMOVE. Just manual analysis and a big ASYNCIFY_WHITELIST !
Yes, this is quite tedious... but that works.
I find propagation by default would be a bit "dangerous", leading to many many things to be included while with a bit of analysis code can be properly fine tuned. But, for doing things quick on small/medium programs, that'd be very helpful I guess. Maybe have an ASYNCIFY_PROPAGATE option ? But that would add a new option... there is already quite a lot of options, so I'm not sure.
|
|
ASYNCIFY_IMPORTS already propagates by default, and it's safe by design, though potentially not quite performance optimal. If through manual analysis you determine a function doesn't need to be instrumented you can use ASYNCIFY_REMOVE which also propagates. Probably the main use of this would be when there's a function that sometimes calls an ASYNCIFY import and sometimes doesn't and you can reliably determine which callers will do which. The main use of ASYNCIFY_ADD is when you use ASYNCIFY_IGNORE_INDIRECT and then you have to add back a few functions which would indirectly call functions which call the imports. And ASYNCIFY_REMOVE is still an option if your analysis leads you to determine some functions never result in an import being called. Not propagating by default is actually the dangerous option. We should be taking advantage of the compiler's static analysis to include all callers. And the REMOVE option is still there to improve performance, but it's a risk that can be taken only when the human has determined it's safe to do so. But without a propagating ADD option you're still required to propagate because all callers must be instrumented, just it's the human who's doing it rather than the compiler. Which is slow and difficult, and silly considering the compiler already has the functionality to do it, we just need to instruct it to. |
|
Well I had some weird cases that under some code path would lead to yield, but under other code paths it would not (emscripten_sleep is called under "if" conditions that occurs only in very specific cases in the overall loop). So static analysis was too eager for the job at the time I did this. I didn't though about the ASYNCIFY_ADD + ASYNCIFY_REMOVE combo, but I guess it was because these options were not existing at the time I did the ports (hence my usage of ASYNCIFY_WHITELIST which is in fact deprecated/replaced by ASYNCIFY_ONLY since some time). So my thoughts on this issue are probably a bit outdated. I am wondering what would happen if a function is found in the propagation list of both REMOVE and ADD (if this is something that may happen) |
|
Good points! Ok, I think I'm convinced that we should accept a PR to modify |
|
I think how it works is:
The REMOVE list propagates through the process, but the ADD list is just appended at the end. Instead it should be added to the list in step 1. Nothing stops a caller of a REMOVE function from being added to the list via another function call. All REMOVE does is stop that single function from being added, stopping the algorithm from walking further from that point, but it's not a promise that its callers will never be instrumented. So if the callers of a REMOVE function can be reached another way they will still be added to the list. If something matches both the ADD and REMOVE lists, that's tricky. I'd say that if it matches a wildcard in one list and a specific name in the other list, whichever is specific should win. I doubt the code currently does that. A specific function name in both lists should be an error. |
|
If we add the |
|
I have a vague memory that I tried adding non-JS functions to the IMPORTS list and something complained, though I'm not certain. I think it does extra processing of the IMPORTS list? Well I just tried it, and nothing complained, but also adding random C functions into ASYNCIFY_IMPORTS doesn't report them or any of their callers in ASYNCIFY_ADVISE. I'm not sure why, but it doesn't seem to have any effect. Btw, if people are working on this stuff, it might be nice to take a look at #13179 too. |
That all sounds right to me.
Sounds right to me too.
Yes, reading it, it has a For simplicity, erroring on any match in both lists, even a wildcard, might be good enough, though I agree it would be nice for a wildcard to be ignored if the other is not a wildcard.
Good point that they are similar, but the list of imports is used for runtime assertions and stuff like that (e.g. to check that only stuff in ASYNCIFY_IMPORTS start a pause, and nothing else). So I think we should keep them separate. If we're agreed on the above then I think we have a plan here. If so, I'd be happy to help with any guidance about how to go about the code for anyone that wants to work on it. |
That sounds risky,.. start with a warning first? |
|
Warning sounds fine to me too. |
|
Since porting my C library to Rust, I now have to disable Could I please ask that the team kindly prioritise this a little higher, to see whether the solution really is as simple as suggested in this comment? #13150 (comment) |
|
@curiousdannii Re-reading the discussion above, I think the solution you propose in that comment would work, but it would be a breaking change. As I mentioned before, that has downsides for people that like the current behavior, which we have gotten reports of. So I think the best path forward is to add a new option, I don't think anyone objects to adding a new option - if someone does please speak up - and if not then what's left is for someone to volunteer to take the time to add such an option in Binaryen and Emscripten. If someone is interested to do so and needs pointers on how to get started, let me know. |
|
Actually wait, there is a PR that seems to be stalled, WebAssembly/binaryen#5935 Let's see if we can get that landed. |
|
Good news, that PR was all ready to land, and I landed it now. To use this right now you can use |
|
Awesome! I've made a PR to add an Emscripten setting, and enabled it by default as I think having it off by default is unsafe. But if the team thinks it shouldn't be on then it can just be switched off. |
This setting affects whether the ASYNCIFY_ADD list propagates, that is, whether it automatically leads to more things added based on the inference. It is on by default, which is safer, but may increase code size in some cases - to undo that, set it to 0. Fixes #13150
) This setting affects whether the ASYNCIFY_ADD list propagates, that is, whether it automatically leads to more things added based on the inference. It is on by default, which is safer, but may increase code size in some cases - to undo that, set it to 0. Fixes emscripten-core#13150
An app I'm porting has 3000 instrumented functions when ASYNCIFY_IGNORE_INDIRECT is disabled, and 100 instrumented functions when it is enabled. Somewhere in between is the actual number of functions that need to be instrumented.
After investigating the code I found one function with indirect calls that does need to be instrumented (there will probably be more), so I specified it in ASYNCIFY_ADD. But then I realised that its status is not propagated upwards, I would need to specify all the functions that call it, etc. I tried adding it to ASYNCIFY_IMPORTS instead, but it was ignored because it's not an import function.
So firstly, have I understood this correctly, that ASYNCIFY_ADD doesn't propagate? (I think ASYNCIFY_REMOVE actually does.) Second, what is the reason for this? Can ASYNCIFY_ADD be changed to propagate? Or could a new setting which propagates be added? I think that would be much more useful and productive than specifying each function in the call tree.
The text was updated successfully, but these errors were encountered: