Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fd_write: Flush on every write to a file #3820

Merged
merged 4 commits into from
May 2, 2023
Merged

Conversation

mucinoab
Copy link
Contributor

Description

Previously we were only flushing the writes until we closed the file, now every write flushes.
This unfortunately will penalize performance, I am open to suggestions on how can this be avoided or minimized.

cc @theduke @syrusakbary @ibrahim-akrab

Fixes #3790
Fixes #3814

/claim #3790

@mucinoab
Copy link
Contributor Author

I want to write a test. Is there any way to examine the state of the env at some point during execution (and not after the execution)?
Here some relevant test I found: stdio.rs test_stdout

@ptitSeb
Copy link
Contributor

ptitSeb commented Apr 25, 2023

Can the flush be done only for stout/stderr, that would avoid the slowdown of flush for regular file @mucinoab

Also, for testing, maybe you try some test that use both stdout and stderr and try to mix both output without/without flushing?

@ibrahim-akrab
Copy link
Contributor

ibrahim-akrab commented Apr 25, 2023

@ptitSeb I mean surely this can be done but then we'll just be waiting for the next issue of someone writing maybe a logger and flushing all the logs to a file before uploading it to some api and not getting all the logs uploaded because wasmer didn't find it suitable to flush the data to the file despite the developer explicitly calling flush.

I think it's not a matter of performance penalization issue as much as program behavior correctness.

@syrusakbary
Copy link
Member

Aside from what was commented, tests are missing. We need to test that this works as we want

@ptitSeb
Copy link
Contributor

ptitSeb commented Apr 26, 2023

I would prefer that we separate issue. This is issue is specifically about stdout/stderr. Generic files are backed up by different virtual-file, that might or might not need flushing. There is a complex and efficient cache system in wasmer, and the point is not to nullify it's effect with this. The solution for generic file might be different, or there might be way to detect a flush.

The fix have to only touch stdout/stderr and not impact other files. If you don't do it, I'll do it after the merge, so I would prefer you do it directly.

@ptitSeb
Copy link
Contributor

ptitSeb commented Apr 28, 2023

you would need to rebase also.

@ibrahim-akrab
Copy link
Contributor

you would need to rebase also.

Done

@ptitSeb ptitSeb enabled auto-merge (squash) May 2, 2023 08:44
@ptitSeb ptitSeb merged commit 796f134 into wasmerio:master May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wasmer stdout is not good fflush doesn't work on WASI
4 participants