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

[3.x] Address slow copy performance when using the FileAccessFilesystemJAndroid implementation #63223

Merged
merged 1 commit into from
Jul 21, 2022

Conversation

m4gr3d
Copy link
Contributor

@m4gr3d m4gr3d commented Jul 20, 2022

Read/write ops for this implementation are done through the java layer via jni, and so for good performance, it's key to avoid numerous repeated small read/write ops due the jni overhead.

The alternative is to allocate a (conversatively-sized) large buffer to reduce the number of read/write ops over the jni boundary.

Fixes #62969

3.x version of #63242

@m4gr3d m4gr3d added bug platform:android topic:core confirmed regression cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release labels Jul 20, 2022
@m4gr3d m4gr3d added this to the 3.5 milestone Jul 20, 2022
@m4gr3d m4gr3d requested a review from a team as a code owner July 20, 2022 03:13
@m4gr3d m4gr3d changed the title Address slow copy performance when using the FileAccessFilesystemJAndroid implementation [3.x] Address slow copy performance when using the FileAccessFilesystemJAndroid implementation Jul 20, 2022
core/os/dir_access.cpp Outdated Show resolved Hide resolved
core/os/dir_access.cpp Outdated Show resolved Hide resolved
@m4gr3d m4gr3d force-pushed the fix_slow_copy_3x branch 2 times, most recently from 406a035 to f0b3ee2 Compare July 20, 2022 21:17
Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

Might be a little more "Godotey" to use a LocalVector local variable for this rather than alloc the buffer directly, but that's more a style / maintenance nitpick, using a buffer directly isn't any worse.

An alternative could be to persist the vector to prevent multiple allocations, but probably won't make a massive difference as file access is expected to be slow anyway, and the approach in the PR avoids tying up any memory.

Also might be a tad better practice to just have the COPY_BUFFER_LIMIT a local const unless it was used in multiple places, to avoid polluting global namespace.

These are all nitpicks though, up to you if you wanted to change imo.

P.S. Haven't tried running it, only looking at the function.

EDIT: Noticed one possible bug though. get_buffer() can return -1 in error condition (maybe this doesn't happen in practice) but might be good to check for this, get_buffer() could change in future. This can probably only currently happen if the malloc fails but could be worth checking that too and ERR_FAILing?

…droid` implementation.

Read/write ops for this implementation are done through the java layer via jni, and so for good performance, it's key to avoid numerous repeated small read/write ops due the jni overhead.

The alternative is to allocate a (conversatively-sized) large buffer to reduce the number of read/write ops over the jni boundary.
@akien-mga akien-mga merged commit c8099e9 into godotengine:3.x Jul 21, 2022
@akien-mga
Copy link
Member

akien-mga commented Jul 21, 2022

Thanks!

@akien-mga akien-mga removed the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Jul 21, 2022
@m4gr3d m4gr3d deleted the fix_slow_copy_3x branch July 22, 2022 03:06
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.

3 participants