console: bound queue to 10 items #6486
Conversation
If you've an application like me (https://github.com/hannesm/maintenance-intent-filter), where nothing ever calls OpamCoreConfig.set/update, you get a pretty large queue. The application consumes a lot of memory. Instead, let's bound the queue size to be 10 (of course, we can discuss the threshold - maybe some other number is better for your use cases)? This partially addresses ocaml#6484
| let b = Buffer.create 128 in | ||
| let timestamp = timestamp () ^ " " in |
There was a problem hiding this comment.
Maybe instead of removing items from the queue without telling anyone, we could just add one when it reaches the threshold that says that the rest hasn't been added. Though it should be a programing error on our part if this ever happens (be it a threshold value too low or something that we haven't thought of happening), so maybe just showing a warning unconditionally instead could be a solution?
| let b = Buffer.create 128 in | |
| let timestamp = timestamp () ^ " " in | |
| let timestamp () = timestamp () ^ " " in | |
| if Queue.length pending = 10 then | |
| Queue.push (1, timestamp (), "truncated logs [...]") | |
| else if Queue.length pending < 10 then | |
| let b = Buffer.create 128 in |
There was a problem hiding this comment.
I've no idea what the purpose of the commit that introduced this queue was, and what the desired semantics are: should the earliest messages be preserved? the latest ones? on which platform(s) does it matter?
depending on the answer to the questions above, I see multiple options: remove the queue entirely; only add the earliest N messages (adding a truncated message is definitively nice); or add the latest N messages (here as well a truncated message would be nice).
Maybe @dra27 who wrote the code years ago can explain his needs when he wrote that code.
There was a problem hiding this comment.
actually thinking about it more, i prefer the solution with the warning. This way you would have seen the need to use OpamCoreConfig.init almost immediately when using the library.
There was a problem hiding this comment.
You mean a warning once when the queue is full (and messages are dropped)? Or a warning for each log message being dropped?
Honestly, from the API I read nowhere that OpamCoreConfig.set/update needs to be called at any time -- and there are sane defaults :) -- so I don't quite understand why there should be a warning, and more fundamentally I don't understand that there's a queue...
There was a problem hiding this comment.
I've no idea what the purpose of the commit that introduced this queue was, and what the desired semantics are:
The purpose is that --cli needs to be evaluated before OpamCoreConfig because some of the environment variable might not be available with some of the values of CLI for example. So a couple of things happens before OpamCoreConfig.init gets run and log gets called. So since the config hasn't been initialized opam doesn't know yet if something has to be shown on screen so those log messages need to be kept until OpamCoreConfig.init is called.
There was a problem hiding this comment.
Thanks for your excellent explanation.
|
Alternative proposal in #6487 |
If you've an application like me
(https://github.com/hannesm/maintenance-intent-filter), where nothing ever calls OpamCoreConfig.set/update, you get a pretty large queue. The application consumes a lot of memory. Instead, let's bound the queue size to be 10 (of course, we can discuss the threshold - maybe some other number is better for your use cases)?
This partially addresses #6484
Please update
master_changes.mdfile with your changes.