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

Yield before flushing io buffers in fsync (sys_fs) #5506

Merged
merged 3 commits into from Mar 8, 2019

Conversation

elad335
Copy link
Contributor

@elad335 elad335 commented Jan 5, 2019

https://blogs.msdn.microsoft.com/oldnewthing/20100909-00/?p=12913
File buffers flushing waits for the disk to push data to the disk, and on windows also waits for the disk to clear its cache.
This is an extremely performance expensive operation, atleast deschedule when doing it.
Thanks for jarves and yahfz for testing and the fix.

Copy link
Contributor

@kd-11 kd-11 left a comment

Choose a reason for hiding this comment

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

Is it really necessary to even log this as warning? Its essentially supposed to be a NOP on a desktop OS and should have no side-effect anyway.

@AniLeo
Copy link
Member

AniLeo commented Jan 5, 2019

You should probably mention this on the comment itself, otherwise no one guarantees some years from now no one will accidentally do that mistake again

@AniLeo
Copy link
Member

AniLeo commented Jan 5, 2019

GOW 3 Demo now loads to menu much faster 👍

@elad335
Copy link
Contributor Author

elad335 commented Jan 5, 2019

@AniLeo Provided the reason for commenting out.

@Nekotekina
Copy link
Member

It's not unnecessary, it's important to ensure consistent state of written files in the case of power loss events and similar disasters. It's not related at all to the write visibility.

The question would be, why it's ever used by a game and what files it's used for.

@elad335
Copy link
Contributor Author

elad335 commented Jan 7, 2019

In gow3 this is used after every 1mb write to a specific file, we can't fix game code logic.

@Nekotekina
Copy link
Member

I'd try to release the thread with lv2_obj::sleep before the sync. There are more things to consider.

@elad335
Copy link
Contributor Author

elad335 commented Jan 7, 2019

how much time do you think the thread should sleep, or only for yield?

@Nekotekina
Copy link
Member

It doesn't need timeout. Just sleep(ppu), it should reschedule after the syscall returns.

@elad335
Copy link
Contributor Author

elad335 commented Jan 7, 2019

Added the requested reschedule/yield.

@Nekotekina
Copy link
Member

I'd like to test it without removing the sync.

@elad335
Copy link
Contributor Author

elad335 commented Jan 7, 2019

are you for real?

@elad335 elad335 closed this Jan 7, 2019
@hch12907
Copy link

hch12907 commented Jan 7, 2019

I wonder if making fsync asynchronous would help.

@elad335
Copy link
Contributor Author

elad335 commented Jan 7, 2019

@Nekotekina Give me one reason the file buffers flushing is still needed in the emulator.

@AniLeo AniLeo requested a review from Nekotekina January 8, 2019 12:04
@elad335 elad335 reopened this Mar 8, 2019
@elad335 elad335 changed the title Remove File buffers flushing from sys_fs Yield before flushing io buffers in fsync (sys_fs) Mar 8, 2019
@AniLeo
Copy link
Member

AniLeo commented Mar 8, 2019

GoW3 Demo loading time improved by a lot, takes only a couple dozen seconds to reach title screen.
Title screen is now up to 18-19 fps from 1-2fps.

Tests on FX8350

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants