-
Notifications
You must be signed in to change notification settings - Fork 87
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
Decrease memory footprint of SessionData values #354
Conversation
Cookie is needed only during HRR and can be cleared afterwards.
Retained memory was unnecessarily large because of two reasons: * one reason was insufficiently evaluated fields sessionCipher and sessionCompression in SessionData values given to the session manager * another one was due to how ByteString-oriented extensions are decoded. ByteString pieces often share the same storage as a large input they are extracted from. Decoded extension values that are held in the context and given to the session manager are now copied to fresh storage in order to break the dependency to the network packet content.
@@ -262,8 +262,9 @@ decodeServerName = runGetMaybe $ do | |||
where | |||
getServerName = do | |||
ty <- getWord8 | |||
sname <- getOpaque16 | |||
let name = case ty of | |||
snameParsed <- getOpaque16 |
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.
What about inserting B.copy
to getOpaque16
(all functions using getBytes
) intead of here?
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.
That would certainly work, but we have also many transient elements, only retained on the stack, or in the context shortly, for which we don't need copying. This is why I clear the cookie value for example.
4f8f014 looks good to me. |
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.
OK for this time.
In my opinion, internal data structures should be |
Merged. |
The report in yesodweb/wai#728 shows a space leak in the session manager but also that retained memory is high overall.
Here are simple mitigations, mainly to decrease what is retained by SessionData values. This kind of modification can probably be carried out further to decrease footprint of live Context values.
With this PR I measure process memory on the server memleak test going down to 24 KB per session instead of 54 KB.
As for design, copying ByteStrings ideally should be moved out of extension decoding but this is a very convenient location (before additional transformations take place, like unpacking to String).