[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.cpp:23
+  return ConstString("arm");
+}
+
----------------
clayborg wrote:
> zturner wrote:
> > clayborg wrote:
> > > One time at startup. No threads contending yet. Asking for plug-in by name is made fast for later. I would leave this.
> > Is asking for a plugin by name something that happens in a hot path?
> No. If we are talking about making the code safer, I would rather go with something that can guarantee safety. The StringRef has the possibility of going wrong if someone uses a local string. So unless we can guarantee it I don't see a reason to change.
I agree with you that it leaves open the possibility of something going wrong, but why isn't "don't use a local string" a sufficient answer to this.  I thought (perhaps mistakenly) that we agreed that if you document your assumptions, you can then assume that people will follow them.  This isn't user input.

It's worth mentioning that strings are one of LLDB's biggest memory hogs, and getting as much stuff out of the global string pool as possible is a win from a memory perspective.  So, I don't think "I'm not really sure if anyone will ever do the wrong thing here, so let's just be safe" is a good long term strategy.  Instead, we shoudl look at each case and say "can we tell users to do something reasonable here", and if so we should do that.

There was a talk at cppcon a few weeks ago from someone at backtrace.io who had some insightful metrics on debugger performance memory consumption, and LLDB had ~2x the memory consumption of GDB.  

So, I think this isn't a pretend problem, and that we should be more vigilant about mindlessly constifying all strings "just in case".  There has to be a strong technical argument for it (i.e. millions of duplicated strings originating from user input, a hot-path, etc).

That said, this is a pretty uninteresting case, so I won't push it here, but unnecessary use of `ConstString` this is something I'm actively interested in reducing.


https://reviews.llvm.org/D31172