Try using fseek before reading a file twice. #1499

branc116 · 2024-06-01T23:03:12Z

I was testing something with vulkan and noticed that some files are being read twice.
I was supprised by that. I started digging and I found this:

commit 992d9cb45f181e747a1a942c6aa09bef94fd9428
Author: Lenny Komow <lenny@lunarg.com>
Date:   Tue Dec 1 10:49:01 2020 -0700

    loader: Remove SEEK_END usage
    
    Change-Id: I699caaf048d70756649d9f6a2c7dfb012c6d2342

     }
-    fseek(file, 0, SEEK_END);
+    // NOTE: We can't just use fseek(file, 0, SEEK_END) because that isn't guaranteed to be supported on all systems
+    do {
+        // We're just seeking the end of the file, so this buffer is never used
+        char buffer[256];
+        fread(buffer, 1, sizeof(buffer), file);
+    } while (!feof(file));
     len = ftell(file);

And then I found this:

Contributor cdotstout commented on Oct 9, 2020
On Fuchsia, at one point we observed a problem where this fseek failed, from loader_get_json in loader.c:

fseek(file, 0, SEEK_END);

We fixed the fseek on Fuchsia so this is not a pressing issue, but we considered switching to fstat instead.

I noted here:
http://www.cplusplus.com/reference/cstdio/fseek/

"Library implementations are allowed to not meaningfully support SEEK_END (therefore, code using it has no real standard portability)"

At least, the fseek return code should probably be checked and an error/warning logged if it fails.

So because of new experimental os, everybody using this loader on linux, bsd, windows, ... has to read each json file 2 times.
Idk, in this pull request I'd just like to use fseek and then if that fails, use double read.

I also did a quick benchmark:

NEW 3924 - 4235 - 18022 (min - avg - max microsecs)
OLD 4440 - 4810 - 19318 (min - avg - max microsecs)

#include <stdio.h>
#include <vulkan/vulkan.h>
#include <stdlib.h>

#define HS (1 << 24)
union {
  size_t data[HS];
  char c_data[HS << 3];
} heap;
size_t heap_index = 0;

void* my_alloc(void* ud, size_t size, size_t alignment, VkSystemAllocationScope allocation_scope) {
  void* ret = &heap.data[heap_index];
  heap_index += (size >> 3) + 1;
  return ret;
}

void* my_realloc(void* ud, void* p, size_t size, size_t alignment, VkSystemAllocationScope allocation_scope) {
  void* ret = &heap.data[heap_index];
  heap_index += (size >> 3) + 1;
  return ret;
}

void my_free(void* ud, void* p) {}
void alloc_notif(void* ud, size_t size, VkInternalAllocationType allocation_type, VkSystemAllocationScope allocation_scope) {}
void free_notif(void* ud, size_t size, VkInternalAllocationType allocation_type, VkSystemAllocationScope allocation_scope) {}

VkAllocationCallbacks allocator = {
  .pUserData = 0,
  .pfnAllocation = my_alloc,
  .pfnReallocation = my_realloc,
  .pfnFree = my_free
};

int main(int argc, char** argv) {
  int n = 1;
  if (argc > 1) {
    n = atoi(argv[1]);
  }
  unsigned long long total = 0;
  unsigned long long m = 1LL<<60LL, M = 0;
  for (int i = 0; i < n; ++i) {
    VkInstanceCreateInfo create_info = {0};
    VkInstance instance = {0};
    unsigned long long cur = 0;
    unsigned long long start = __builtin_ia32_rdtsc();
    vkCreateInstance(&create_info, &allocator, &instance);
    unsigned long long end = __builtin_ia32_rdtsc();
    cur = end - start;
    total += cur;
    m = m < cur ? m : cur;
    M = M < cur ? cur : M;
    heap_index = 0;
  }
  printf("%llu - %llu - %llu\n", m / 1000, total / n / 1000, M / 1000);
  return 0;
}

// cc -L $PWD/build/loader -lvulkan test.c && strace ./a.out 2> tmp
// cc -O3 -L $PWD/build/loader -lvulkan ../test.c && LD_LIBRARY_PATH="$PWD/loader" ./a.out

ci-tester-lunarg · 2024-06-01T23:03:22Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

CLAassistant · 2024-06-01T23:03:24Z

All committers have signed the CLA.

ci-tester-lunarg · 2024-06-01T23:03:25Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

ci-tester-lunarg · 2024-06-10T18:40:04Z

CI Vulkan-Loader build queued with queue ID 198556.

ci-tester-lunarg · 2024-06-10T18:42:09Z

CI Vulkan-Loader build # 2597 running.

charles-lunarg

Very good PR! Yes, I concur doing a double read is a bit pointless if not performance intensive.

Definitely something which looks like a good suggestion. There is a current hole in the CI coverage for Windows platforms. But that can be ignored because this code is inside UNIX only codepaths.

ci-tester-lunarg · 2024-06-10T18:45:48Z

CI Vulkan-Loader build # 2597 passed.

charles-lunarg · 2024-06-10T18:48:08Z

Ah yes, about that "hole in CI". The fix for the failing windows CI checks is to rebase on main. The fix for the clang-format check is to run your change through it (or just copy what the CI check expected the code to be formatted as, since that will be the same result).

gpx1000 · 2024-06-10T19:12:01Z

FWIW, can probably just fall back on using fstat?

branc116 · 2024-06-11T14:31:45Z

FWIW, can probably just fall back on using fstat?

Initially I changed it this function to use fstat, but then googled and found out that fstat is posix standard and Idk what are the differences between Bsds and linux.
Then I looked for inspiration in other peoples code. Specifically, stb... His implementation also uses fseek, and who am I to question him. xD

Ah yes, about that "hole in CI". The fix for the failing windows CI checks is to rebase on main.

Did this.

The fix for the clang-format check is to run your change through it (or just copy what the CI check expected the code to be formatted as, since that will be the same result).

Also did this. I haven't noticed 4 spaces is used for indentation...

but that can be ignored because this code is inside unix only codepaths.

I don't think it is. It looks like windows is also calling to this. There is an ifdef Windows in the same function -> that means someone expected for this function to be called on windows. Or am I missing something?

ci-tester-lunarg · 2024-06-11T14:32:39Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

ci-tester-lunarg · 2024-06-11T14:32:40Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

ci-tester-lunarg · 2024-06-11T14:34:39Z

CI Vulkan-Loader build queued with queue ID 199158.

ci-tester-lunarg · 2024-06-11T14:35:01Z

CI Vulkan-Loader build # 2598 running.

charles-lunarg · 2024-06-11T14:36:06Z

Yep, it is I who misread the code. Yes, this code runs on all platforms, so the lack of windows CI coverage is a bit worrysome.
Hmm if there isn't great consensus on how to do this, I'll need to do a little research before pulling the merge trigger. Just as a sanity check for my own knowledge.

ci-tester-lunarg · 2024-06-11T14:38:42Z

CI Vulkan-Loader build # 2598 passed.

gpx1000 · 2024-06-11T16:31:04Z

fstat works in all bsds that I'm aware of including FreeBSD which is Very similar to OSX and why it works in Apple too without change.
Windows would be done with GetFileSizeEx.

However, another option is to use: https://en.cppreference.com/w/cpp/filesystem/file_size

charles-lunarg · 2024-06-11T16:37:09Z

fstat would appear to me as the most straight forward path.

I can unilaterally state that anything requiring C++ is a no-go. Not out of ideological issues, but because of requiring C++ would change the required dynamic libraries. In the future, that might change, but not for this type of change.

ci-tester-lunarg · 2024-06-12T16:51:42Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

ci-tester-lunarg · 2024-06-12T16:51:42Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

branc116 · 2024-06-12T16:53:49Z

With my first commit my goal was primarily to keep compatibility across os-es and to improve performance ( reduce syscalls )
But now you prompted me to reduce syscall even further and to keep compatibility across os-es more or less the same.
In this new commit I introduce new function called read_entire_file.
This function should read the entire file optimally ( using least number of syscalls possible. )
I tested this on my linux machine and it works.

First question is if windows implementation will work

I think it should work, but windows unit tests should be enabled and someone should test if it works on windows.
- I don't really have access to windows machine. And I don't know if vulkan and vm work together...

Second question is if this is optimal windows implementation...

I have even less insight into that..

Third question is if this will work on other COMMON_UNIX_PLATFORMS

I don't know what are all the targets that Vulkan-Loader targets
Posix function that need checking is:
- open
- fstat
- read
It'd be cool if we had a table what posix functions are implemented in any posix os-es.

I'd not be comfortable with you pushing this until windows ci is working and until we find posixOS/function table to check everything works...

ci-tester-lunarg · 2024-06-12T16:54:02Z

CI Vulkan-Loader build queued with queue ID 199730.

ci-tester-lunarg · 2024-06-12T16:55:01Z

CI Vulkan-Loader build # 2601 running.

ci-tester-lunarg · 2024-06-12T16:56:15Z

CI Vulkan-Loader build # 2601 failed.

branc116 · 2024-06-12T18:02:31Z

Damn, didn't expect so many failed test cases.. I'll look at it tomorrow.

Good tests :)

ci-tester-lunarg · 2024-06-13T14:09:58Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

ci-tester-lunarg · 2024-06-13T14:09:58Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

ci-tester-lunarg · 2024-06-13T14:20:33Z

CI Vulkan-Loader build queued with queue ID 200294.

ci-tester-lunarg · 2024-06-13T14:25:01Z

CI Vulkan-Loader build # 2603 running.

ci-tester-lunarg · 2024-06-15T09:38:44Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

ci-tester-lunarg · 2024-06-15T09:38:44Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

ci-tester-lunarg · 2024-06-15T12:05:00Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

ci-tester-lunarg · 2024-06-15T12:05:00Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

ci-tester-lunarg · 2024-06-15T12:24:05Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

ci-tester-lunarg · 2024-06-15T12:24:05Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

charles-lunarg

More style changes but the meat of the code looks good otherwise. Style is to not mix assignment and comparisons in the same line. Just makes everything more consistent.

charles-lunarg · 2024-06-17T17:30:01Z

loader/cJSON.c

+    assert(json != NULL);
+
+    *json = NULL;
+    if (VK_SUCCESS != (res = loader_read_entire_file(inst, filename, &json_buf))) goto out;


Suggested change

if (VK_SUCCESS != (res = loader_read_entire_file(inst, filename, &json_buf))) goto out;

res = loader_read_entire_file(inst, filename, &json_buf);

if (VK_SUCCESS != res) {

goto out;

}

charles-lunarg · 2024-06-17T17:30:27Z

loader/cJSON.c


-    // Can't be a valid json if the string is of length zero
-    if (len == 0) {
+    if (NULL == (file = fopen(filename, "rb"))) {


Suggested change

if (NULL == (file = fopen(filename, "rb"))) {

file = fopen(filename, "rb");

if (NULL == file) {

charles-lunarg · 2024-06-17T17:31:34Z

loader/cJSON.c

+        res = VK_ERROR_INITIALIZATION_FAILED;
+        goto out;
+    }
+    if (NULL == (*out_buff = (char *)loader_instance_heap_calloc(inst, stats.st_size + 1, VK_SYSTEM_ALLOCATION_SCOPE_COMMAND))) {


Suggested change

if (NULL == (*out_buff = (char *)loader_instance_heap_calloc(inst, stats.st_size + 1, VK_SYSTEM_ALLOCATION_SCOPE_COMMAND))) {

*out_buff = (char *)loader_instance_heap_calloc(inst, stats.st_size + 1, VK_SYSTEM_ALLOCATION_SCOPE_COMMAND);

if (NULL == *out_buff) {

ci-tester-lunarg · 2024-06-20T08:12:48Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

ci-tester-lunarg · 2024-06-20T08:12:48Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

ci-tester-lunarg · 2024-06-20T15:09:19Z

CI Vulkan-Loader build queued with queue ID 204134.

ci-tester-lunarg · 2024-06-20T15:10:01Z

CI Vulkan-Loader build # 2615 running.

ci-tester-lunarg · 2024-06-20T15:13:31Z

CI Vulkan-Loader build # 2615 failed.

ci-tester-lunarg · 2024-06-20T17:06:26Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

ci-tester-lunarg · 2024-06-20T17:06:26Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

fopen is overriden and open is not, so fopen needs to be used if you want test to pass...

ci-tester-lunarg · 2024-06-20T17:08:57Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

ci-tester-lunarg · 2024-06-20T17:08:57Z

Author branc116 not on autobuild list. Waiting for curator authorization before starting CI build.

branc116 · 2024-06-20T17:13:56Z

I changed it :)

charles-lunarg approved these changes Jun 10, 2024

View reviewed changes

branc116 force-pushed the main branch from 8cfaf52 to a52b0b4 Compare June 11, 2024 14:32

charles-lunarg reviewed Jun 17, 2024

View reviewed changes

branc116 force-pushed the main branch from 5d5b65b to 7b4e0cd Compare June 20, 2024 08:12

branc116 force-pushed the main branch from 7b4e0cd to 4a7fcdc Compare June 20, 2024 17:06

branc116 added 10 commits June 20, 2024 19:08

Try using fseek before reading a file twice.

f548c3d

Fix format.

05bf6b9

Added loader_read_entire_file function in cJSON.

26fac01

Fix windows build.

67dc001

Fixed unix tests.

61bc8fd

fopen is overriden and open is not, so fopen needs to be used if you want test to pass...

Fixed formating.

a61c76e

Fix format.

cb9a133

Fix windows file read.

65b408e

Fix windows file read.

5464708

Style fix.

476e363

branc116 force-pushed the main branch from 4a7fcdc to 476e363 Compare June 20, 2024 17:08

	if (NULL == (file = fopen(filename, "rb"))) {
	file = fopen(filename, "rb");
	if (NULL == file) {

	if (NULL == (out_buff = (char )loader_instance_heap_calloc(inst, stats.st_size + 1, VK_SYSTEM_ALLOCATION_SCOPE_COMMAND))) {
	out_buff = (char )loader_instance_heap_calloc(inst, stats.st_size + 1, VK_SYSTEM_ALLOCATION_SCOPE_COMMAND);
	if (NULL == *out_buff) {

Try using fseek before reading a file twice. #1499

Are you sure you want to change the base?

Try using fseek before reading a file twice. #1499

Conversation

branc116 commented Jun 1, 2024

ci-tester-lunarg commented Jun 1, 2024

CLAassistant commented Jun 1, 2024 • edited Loading

ci-tester-lunarg commented Jun 1, 2024

ci-tester-lunarg commented Jun 10, 2024

ci-tester-lunarg commented Jun 10, 2024

charles-lunarg left a comment

Choose a reason for hiding this comment

ci-tester-lunarg commented Jun 10, 2024

charles-lunarg commented Jun 10, 2024

gpx1000 commented Jun 10, 2024

branc116 commented Jun 11, 2024

ci-tester-lunarg commented Jun 11, 2024

ci-tester-lunarg commented Jun 11, 2024

ci-tester-lunarg commented Jun 11, 2024

ci-tester-lunarg commented Jun 11, 2024

charles-lunarg commented Jun 11, 2024

ci-tester-lunarg commented Jun 11, 2024

gpx1000 commented Jun 11, 2024

charles-lunarg commented Jun 11, 2024

ci-tester-lunarg commented Jun 12, 2024

ci-tester-lunarg commented Jun 12, 2024

branc116 commented Jun 12, 2024

ci-tester-lunarg commented Jun 12, 2024

ci-tester-lunarg commented Jun 12, 2024

ci-tester-lunarg commented Jun 12, 2024

branc116 commented Jun 12, 2024

ci-tester-lunarg commented Jun 13, 2024

ci-tester-lunarg commented Jun 13, 2024

ci-tester-lunarg commented Jun 13, 2024

ci-tester-lunarg commented Jun 13, 2024

ci-tester-lunarg commented Jun 15, 2024

ci-tester-lunarg commented Jun 15, 2024

ci-tester-lunarg commented Jun 15, 2024

ci-tester-lunarg commented Jun 15, 2024

ci-tester-lunarg commented Jun 15, 2024

ci-tester-lunarg commented Jun 15, 2024

charles-lunarg left a comment

Choose a reason for hiding this comment

charles-lunarg Jun 17, 2024

Choose a reason for hiding this comment

charles-lunarg Jun 17, 2024

Choose a reason for hiding this comment

charles-lunarg Jun 17, 2024

Choose a reason for hiding this comment

ci-tester-lunarg commented Jun 20, 2024

ci-tester-lunarg commented Jun 20, 2024

ci-tester-lunarg commented Jun 20, 2024

ci-tester-lunarg commented Jun 20, 2024

ci-tester-lunarg commented Jun 20, 2024

ci-tester-lunarg commented Jun 20, 2024

ci-tester-lunarg commented Jun 20, 2024

ci-tester-lunarg commented Jun 20, 2024

ci-tester-lunarg commented Jun 20, 2024

branc116 commented Jun 20, 2024

CLAassistant commented Jun 1, 2024 •

edited

Loading