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

Make all file access 64-bit #27247

Closed

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Mar 19, 2019

Bugsquad edit: Superseded by #48768 and #47254

DISCLAIMER:
This change is big and can break a lot of stuff. I'm marking it as [wip] until I have done everything needed I can think of.
Maybe it should go directly to 4.0.
UPDATE:
This changes a lot of types and may introduce bugs, but it shouldn't break compatibility. Maybe it's suitable for 3.2 or even 3.1.1?

This is all I can do so far on my side. Now I'm asking the community for testing and feedback.
UPDATE: As soon as I get the build checks to pass. :P CI checks passing!


UPDATE: Current commit message after modifications:

This changes the types of a big number of variables.

General rules:

  • Using int64_t in general. With 64-bit we no longer need to use unsigned type. That matches Variant better and avoids akward casts. Checks for negative numbers are performed where they don't make sense (seek, read/write buffer size).
  • Previous point means no more size_t for file size/offsets.
  • Not worrying about FileAccess::get_64/store_64 working with unsigneds. I haven't found any place where conversion to unsigned will cause trouble and saves us from having to add get/store_64_signed or similar.
  • Using int32_t integers for concepts not needing such a huge range, like pages, blocks, etc.

In addition:

  • Improve usage of integer types in some related places; namely, DirAccess, core binds.
  • The binding for Directory::get_space_left() tried to return the size in MiB, but the expression was missing parentheses, so the 1024 * 1024 is removed and it keeps working as usual, returning bytes.

Fixes #27168.
Bugsquad edit: Fixes godotengine/godot-proposals#400.

@RandomShaper RandomShaper added this to the 3.2 milestone Mar 19, 2019
@RandomShaper RandomShaper requested review from hpvb and reduz as code owners March 19, 2019 20:48
@RandomShaper RandomShaper force-pushed the file-access-64-bit branch 11 times, most recently from c705543 to d5b6d5f Compare March 26, 2019 18:50
@RandomShaper RandomShaper force-pushed the file-access-64-bit branch 2 times, most recently from 18f6b83 to fd014e7 Compare March 27, 2019 00:01
@RandomShaper RandomShaper requested a review from karroffel as a code owner March 27, 2019 00:01
@RandomShaper RandomShaper force-pushed the file-access-64-bit branch 2 times, most recently from 93e6c5e to 2baeec2 Compare March 30, 2019 00:21
@RandomShaper RandomShaper changed the title Make all file access 64-bit [wip] Make all file access 64-bit Mar 30, 2019
@hpvb
Copy link
Member

hpvb commented Apr 22, 2019

This looks good to me. But I don't think this can go into 3.1 due to gdnative compatibility.

@akien-mga
Copy link
Member

Needs a rebase, otherwise this should be good to merge IMO to get some broader testing, if @reduz agrees.

This changes the types of a big number of variables.

General rules:
- Using `int64_t` in general. With 64-bit we no longer need to use unsigned type. That matches `Variant` better and avoids akward casts. Checks for negative numbers are performed where they don't make sense (seek, read/write buffer size).
- Previous point means no more `size_t` for file size/offsets.
- Not worrying about `FileAccess::get_64/store_64` working with unsigneds. I haven't found any place where conversion to unsigned will cause trouble and saves us from having to add `get/store_64_signed` or similar.
- Using `int32_t` integers for concepts not needing such a huge range, like pages, blocks, etc.

In addition:
- Improve usage of integer types in some related places; namely, `DirAccess`, core binds.
- The binding for `Directory::get_space_left()` tried to return the size in MiB, but the expression was missing parentheses, so the `1024 * 1024` is removed and it keeps working as usual, returning bytes.

Fixes godotengine#27168.
@aaronfranke
Copy link
Member

What kind of >2 GB files are expected to work? I tried loading a 2.4 GB PNG file, and it did not work, the editor crashes when it tries to import. Ubuntu 18.04 64-bit, compiled with this PR.

ERROR: alloc_static: Condition ' !mem ' is true. returned: __null  At: core/os/memory.cpp:87.

However, every project that works before still works, so I don't notice any regressions. I think that any issues would only be noticed after merging and lots of people tested it.

@RandomShaper
Copy link
Member Author

Maybe some libraries aren't 64-bit aware, so this PR will make some file types "addressable" inside a big .pck, but they'll still be not readable if bigger than 2 (or 4) GiB.

However, It's good news that you haven't found regressions. And then I agree with you in that it would be good to take advantage of the time span before 3.2 to spot errors.

@akien-mga
Copy link
Member

What kind of >2 GB files are expected to work? I tried loading a 2.4 GB PNG file, and it did not work, the editor crashes when it tries to import. Ubuntu 18.04 64-bit, compiled with this PR.

AFAIK this PR is to allow making PCKs of more than 2 GB, but not necessarily having a single file of this size in the PCK.

Try to compute how much VRAM is necessary to load a 2.4 GB PNG and you should see why you ran out of memory ;)

@akien-mga
Copy link
Member

Needs a rebase, otherwise I would also like to merge this ASAP.

@akien-mga
Copy link
Member

Some comments from @reduz on IRC:

14:47 <reduz> Akien: dont merge that one, wait for the 4.0 branch to do those changes
14:52 <reduz> Akien: also afaik the right type for files isnt offset_t? maybe TMM knows a bit better about this
14:54 <reduz> Akien: I'm also sure C++11 has a standard type for file offsets (off_t), so when we do the move to it, we can use it

So I guess this will have to wait for 4.0.

@akien-mga akien-mga modified the milestones: 3.2, 4.0 Jun 12, 2019
@akien-mga akien-mga added the bug label Jun 12, 2019
@RandomShaper
Copy link
Member Author

RandomShaper commented Jun 12, 2019

The idea behind using int64_t for all sizes and offsets is an attempt to make the I/O API friendlier, more Godotish, if that makes sense. Some may argue that this is a case of primitive type fever (or something similar).

I think this is cleaner, leaving to the I/O implementations the work of dealing with off_t, ssize_t and how they are defined depending on the target machine and preprocessor definitions, exposing instead a simpler API.

Before making this I researched and tried to find something that would fit in Godot, while being correct on all platforms. Of course, my point of view is debatable and I may be wrong on some of my assumptions.

@Anutrix
Copy link
Contributor

Anutrix commented Feb 14, 2020

@RandomShaper Can you try rebasing? Looking forward to this.

@aaronfranke
Copy link
Member

@RandomShaper This feature is awesome, but it needs to be rebased on the latest master branch. If you can find the time, please do so.

@RandomShaper
Copy link
Member Author

Yes, this PR is quite old. I guess the best thing would be doing it from scratch given the sheer amount of changes that have happened since.

@aaronfranke
Copy link
Member

Since this PR has not been updated in a long time and may need to be redone from scratch, I'm closing this.

I'll add the "Salvageable" tag. I think @RandomShaper still wants to be the one redoing this, but anyone is welcome to.

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