Added a system property, that enables the zip file cache for larger f… #2
Conversation
| if (fs == null) { | ||
| fs = FileSystems.newFileSystem(uri, Collections.emptyMap()); | ||
| Map<String, Object> env = new HashMap<>(); | ||
| if(System.getProperty("tiny_use_zipcache","FALSE").equals("TRUE")){ |
There was a problem hiding this comment.
Do people use lowercase or uppercase for values more often?
There was a problem hiding this comment.
Better yet, use lowercase and call toLowerCase on the property value. Furthermore, System.getProperty is null if the property isn't present, so this will crash with an NPE if the property isn't specified.
There was a problem hiding this comment.
It can't do that, because I give a default value to be used when it's not present. I can set it to lowercase.
Although, it's only a partial fix, because it still runs out of memory, but it's no longer because of the zipfs.
There was a problem hiding this comment.
My suggestion is to use "tiny.useZipCache"
Also - don't forget Boolean.getBoolean() 😉
|
Can you elaborate on the benefits? Any measurements? |
|
It crashes on our ci server when it tries to load up the entire archive, as it runs out of memory. We can't give it more. Modmuss benchmarked it, the entire gradle build should use 1.1G, but 1.5 is not enough. It fixes that issue, but then it starts to do the same as it makes byte buffers that are too large, So we might need a different fix. |
fix mappings propogation from merge
…iles.
If I understood it correctly, than line 253 recieves the uri of the jar file, which turns into a zipfilesystem under the hood.
I added a system property to set the proper environmental values. It's a system property, so that people who don't need it don't need to bother, and to make it plug into loom as is, without modifications.
CI servers can set the property when required.