royitaqi wrote:

Hi @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:
1. 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.
2. 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.
3. 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.

**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.
1. Prematurely add settings (which we cannot remove, I assume) can lead us down 
to a more difficult road.
2. 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`.
3. 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.
4. (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.


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

Reply via email to