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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
