-
Notifications
You must be signed in to change notification settings - Fork 74
Unnecessary file reloading issue slow 318 #323
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
Unnecessary file reloading issue slow 318 #323
Conversation
| } | ||
|
|
||
| fclose(h); | ||
| fclose(oh); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this c code was already significantly better then what an applicant for senior firmware engineer wrote in London 😅
now it's even better, thank you 🙏🏽
| removeBundleFromQueue loads | ||
| (input, action) <- liftIO $ getInputForBundle bundle | ||
| perform input action | ||
| when runable $ do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this check? and what happens if it's not a gzip/runnabble?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I can add puml files to this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh wow!
hmmm, is it possible to link from the comment to this puml file? It's beneficial.
great work, thanks 🙏🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ulidtko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decent workaround, and good diagram! Please add it too.
Two comments.
-
This wires in a hard coupling / dependency on bundles being gzipped. Not uncompressed, not bzip2, not xz, not zst.
- It's OK for now, as keter hardcodes a dependency onto gzip too:
keter/src/Keter/TempTarball.hs
Line 61 in ee4c968
let entries = Tar.read $ decompress lbs - but will make adding other compression algos more difficult, would that ever become desired.
- It's OK for now, as keter hardcodes a dependency onto gzip too:
-
I think we should not decompress while retaining the entire thing in memory (both the archive, and the resulting tar file). It's a no-brainer use-case for conduits, which Keter already depends on. See inline
src/Keter/AppManager.hs
Outdated
| isGzip :: FilePath -> KeterM AppManager Bool | ||
| isGzip bundle = do | ||
| eres <- liftIO $ E.try @SomeException $ do | ||
| content <- B.concat . L.toChunks <$> fmap decompress (L.readFile bundle) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can get pretty heavy on RAM.
Bundles can get ~200 MB compressed... and more uncompressed... It's a pity to use so much memory just to get a Bool!
And keter already has conduit-extra in dependencies. So it'd be pretty simple:
import Conduit
import Data.Conduit.Zlib as Zlib
isGzip :: FilePath -> IO Bool
isGzip bundle =
either (const False) (const True) =<< E.try @SomeException (
runConduitRes $
Conduit.sourceFile bundle
.| Gzip.ungzip
.| sinkNull
)... and will process the file in constant memory, never retaining more than 1 chunk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that this example always shows true on gzip files. To check full file loading I've add readSize and countSize steps. It leads to run it in State monad.
a5426fe to
07e8f6c
Compare
|
thanks 🙏🏽 |

No description provided.