jimingham wrote:
> On May 7, 2024, at 11:28 AM, royitaqi ***@***.***> wrote: > > > Hi @jimingham <https://github.com/jimingham> , > > Sorry to come in a little late on this > > No worries. Thanks for chiming in. > > I think we need a setting to turn this off as well. If you aren't planning to > use the transcript, you shouldn't have to pay the cost for it. > > Could you elaborate about why this is necessary? If we really need this, can > I add the setting in a follow-up PR? > > -- > > FWIW, to help make the conversation faster (reduce rounds of communication), > I will write down my gut feeling below. Please kindly correct me if I'm wrong. > > First, why I think the added cost is negligible: > > The baseline is that there is already a m_transcript_stream, which doesn't > have any settings to turn off. This PR just doubles that cost (plus some > string split operations). It doesn't add huge cost on top of this baseline. That's equally an argument for allowing me to turn that off as well - if I never plan to use `session save` then it's silly to take up memory for that either. > Maintaining transcript is a per-command operation, and commands are > relatively infrequently invoked. Here I assume the common use cases run tens > or at most hundreds of commands in a debug session. The above cost times 10x > ~ 100x isn't much. People have automated scripts that run debug sessions, and you are also storing output text so this can get fairly big. Plus if you start adding time stamps we're stopping to get the system clock on every command, it gets more expensive. For people running lldb on big beefy machines, this is unlikely to matter for, but people also run lldb on low memory devices, where resources are tighter. > The cost of the commands themselves (and the process) are much higher than > that of the transcript. E.g. resolving breakpoints and printing variables all > need to do actual work, rather than just recording info. I don't think "unrelated thing X is bigger" is a a useful data point when considering whether it's good to do Y. > Secondly, if we are going to add settings, there needs to be product design > considerations based on user usage/feedback, so we probably need more time to > gather data points after shipping this feature. > > Prematurely add settings (which we cannot remove, I assume) can lead us down > to a more difficult road. Not sure why a setting to turn off recording transcripts would ever be controversial. > I think it will be the norm that we record transcripts, like bash/zsh records > command histories, so by default this should be on. In fact it is already, by > m_transcript_stream. I guess. I must admit I've used lldb since it existed and have never once used `session save`. After all, my Terminal has a nice big buffer... For my workflow that's all wasted memory. > Besides just a binary on/off, settings can also allow more granular control. > E.g. verbosity; only log transcript for a subset of commands; limit the > memory footprint by using a cyclic buffer; etc. If we add these settings, > they will cause the on/off setting to be redundant and needs to be removed. I don't see the force of this argument at all. Having a control "whether to do X at all" and then another set "if I were to do X, how should I do it" is a pretty standard approach. As a matter of fact, that's a better design since then I can disable the feature temporarily without having to mess up the settings I want when I do do it. > (As mentioned in "Secondly") The above are all possibilities. We need user > usage/feedback to guide the design of these settings. So I think we should > ship this PR first then gather data points. I really don't think this is controversial, and I'm not sure how you would gather data points. If it helps to get started, however, I vote there should be a way to turn this off. But if you are running out of time for this feature and really need to get it off your plate, deferring the ability to turn it off to a subsequent PR is okay. Jim > — > Reply to this email directly, view it on GitHub > <https://github.com/llvm/llvm-project/pull/90703#issuecomment-2099053680>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/ADUPVW5NYBNLLRY6ETNKPN3ZBEMMFAVCNFSM6AAAAABHBPBMWOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJZGA2TGNRYGA>. > You are receiving this because you were mentioned. > https://github.com/llvm/llvm-project/pull/90703 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
