[file_packager] Split data files when file size exceeds ArrayBuffer limit#24802
[file_packager] Split data files when file size exceeds ArrayBuffer limit#24802arsnyder16 wants to merge 32 commits intoemscripten-core:mainfrom
Conversation
|
@sbc100 Is this aligned with what you were thinking? |
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
Can we add some tests for this? What are the specific limits you are running into? Are those limits not fixed? i.e. does it need to be configurable?
I wanted to make sure i was on the right track before adding tests. |
|
This does seem like a reasonable approach. I'm still a little fuzzy on exactly why and when this might be needed in the real world, I think I need to go re-read our original discussion, but I also think including more information in the PR (i.e. in the description, or in comments) would be good. |
Updated the description, based on that if you want me to hard code the limit into the package i can take that approach |
…ackages # Conflicts: # tools/file_packager.py
arsnyder16
changed the title
|
@sbc100 Going to look at adding tests for this. Do you have an recommendations? Would you like me to generate temporary files (s) so that i can reach the 2Gi limit? |
|
@sbc100 This should be ready for review, not sure about the test failure if they are flaky or if its related to my change. I don't see them fail locally. |
|
Peculiarly I find that in Chrome, the max ArrayBuffer limit is not 2GB (2048MB), but 2046MB: 
Firefox supports up to 16GB ArrayBuffers: 
In node.js I get up to 4GB ArrayBuffers: 
What is your target environment? |
|
@juj Thanks! My target is chrome, i incorrectly associated in the ArrayBuffer documentation Looks like Chrome has the lowest limit, i will change to use that limit. Safari appears to be 4Gi |
arsnyder16
changed the title
…ackages # Conflicts: # tools/file_packager.py
…nyder16/emscripten into asnyder/split_large_packages
…ackages # Conflicts: # tools/file_packager.py
| proc = self.run_process([FILE_PACKAGER, 'test.data', '--preload', 'huge.dat'], check=False, stdout=PIPE, stderr=PIPE) | ||
| self.assertEqual(proc.returncode, 1) | ||
| self.assertContained('error: cannot package file greater than 2046 MB does not exist', proc.stderr) | ||
| self.clear() |
There was a problem hiding this comment.
It looks like the above tests verify that file packager does split up files, though there is no functional test to verify that e.g. the bytes were split appropriately, and that the final end result loads up properly? Would that be important to test?
There was a problem hiding this comment.
I am not sure how critical it is, looks like all the file_packager logic is part of these unit tests, seems like out of scope for this PR to add tests that verify the loading of the file_packager results
There was a problem hiding this comment.
There are many tests that verify load of file packager results. Basically any test that uses emcc with --preload-file is verifying this.
There are also tests that call FILE_PACKGER directly and then execute the result. See test_file_packager_separate_metadata for example.
There was a problem hiding this comment.
@sbc100 Thanks for pointing out these decks, test_file_packager_separate_metadata was a good example to follow to test this logic
…ackages # Conflicts: # test/test_other.py # tools/file_packager.py
sbc100
changed the title
3 participants
Currently ArrayBuffer cannot be larger than 2046 on chrome. So bundling a large amount of files using file_packager.py the
.datawill not be allowed to load. Breaking up files into multiple data files by passes this issue.Note this was a solution proposed for issues described in this thread #24691