On Thu, Jul 15, 2021 at 1:20 AM Chris Johns <chr...@rtems.org> wrote:
>
> On 15/7/21 12:45 pm, Alex White wrote:
> > On Tue, Jul 6, 2021 at 7:55 PM Chris Johns <chr...@rtems.org> wrote:
> >>
> >> On 3/7/21 5:56 am, Alex White wrote:
> >> > On Wed, Jun 30, 2021 at 11:40 PM <chr...@rtems.org> wrote:
> >> >>
> >> >> From: Chris Johns <chr...@rtems.org>
> >> >>
> >> >> - The member variable `path_` cannot be a reference and initialised to
> >> >> a const char* type input. To do so would require there is a temporary
> >> >> with
> >> >> an unspecified life time.
> >> >> ---
> >> >> tester/covoar/AddressToLineMapper.h | 2 +-
> >> >> tester/covoar/Target_aarch64.h | 2 +-
> >> >> 2 files changed, 2 insertions(+), 2 deletions(-)
> >> >>
> >> >> diff --git a/tester/covoar/AddressToLineMapper.h
> >> > b/tester/covoar/AddressToLineMapper.h
> >> >> index 88bf475..c78aef7 100644
> >> >> --- a/tester/covoar/AddressToLineMapper.h
> >> >> +++ b/tester/covoar/AddressToLineMapper.h
> >> >> @@ -84,7 +84,7 @@ namespace Coverage {
> >> >> * An iterator pointing to the location in the set that contains
> >> >> the
> >> >> * source file path of the address.
> >> >> */
> >> >> - const std::string& path_;
> >> >> + const std::string path_;
> >> >
> >> > Ryan alerted me about this issue a couple weeks back. This patch would
> >> > fix the
> >> > problem, but it would lead to a second copy of every file path string
> >> > being
> >> > stored in memory. This would also take away the usefulness of the set of
> >> > file
> >> > path strings maintained by the AddressLineRange class.
> >> >
> >> > Instead, I propose we change the empty SourceLine constructor to take a
> >> > `const
> >> > std::string&`. This would allow the addition of something like this to
> >> > the top
> >> > of AddressToLineMapper::getSource():
> >> > const std::string unknown = "unknown";
> >> > const SourceLine default_sourceline = SourceLine(unknown);
> >> >
> >> > That should eliminate the issue and preserve the memory conservation
> >> > efforts of
> >> > the original design.
> >>
> >> Yes it would but for some reason, that I cannot put my finger on, it seems
> >> like
> >> a breach of boundaries between classes.
> >>
> >> How much data are we talking about? Are you able to see the memory foot
> >> print
> >> with the strings being contained vs what you have now?
> >
> > Sorry for the late reply.
> >
> > What I have now yields a peak memory usage for covoar when run on all ARM
> > tests
> > of 219 MB.
> > Changing `SourceLine::path_` to plain `std::string` results in an increase
> > to
> > 523 MB.
> >
> > So it is a significant increase.
>
> Yes and thank you for the test results. This makes sharing a worth while
> exercise.
>
> >> If the figures show the strings need to be shared to avoid a memory blow
> >> out
> >> would a std::shared_ptr<std::string> be something to look at where all
> >> references are a shared pointer? A shared pointer means any changes do not
> >> flow
> >> from one class to other.
> >
> > I don't think that `std::shared_ptr` would help here. Wouldn't that handle
> > the
> > case of an unknown path the same way as a raw pointer solution? Wouldn't we
> > still have to check the return value of `SourceLine::path()` to make sure
> > that
> > it is not null?
> > The only other way I see to make it work would be to store some "unknown"
> > string
> > object and hand pointers to it to all `SourceLine` objects with unknown
> > paths.
> > But that seems mostly equivalent to what I propose in my original reply to
> > this
> > patch.
>
> I have not checked all the detail but a raw pointer will always be fragile
> compared to a shared_ptr. You may get it working but there is nothing making
> sure any references are still pointing to valid memory. Given the difference
> in
> sizes you found there is a lot of sharing and so a lot of references.
>
> > Now that I think about it, maybe making `SourceLine::_path` into an
> > `std::string*` and doing a `nullptr` check in the `SourceLine::path()`
> > would be
> > most elegant?
>
> All I see are potential leaks, maybe not now but may be after the next person
> makes changes.
I don't see how potential leaks could happen with this. The issue of a possible
dangling pointer, which I can see, is eliminated by design.
`AddressLineRange` owns the `std::string` vector.
`AddressLineRange` also owns the `SourceLine` vector. `SourceLine` owns the
`path_` that references a string in the above vector.
This means that the strings will only be deleted when `AddressLineRange` is
deleted. At the same time, all of the `SourceLine` objects will be deleted. So,
unless I'm missing something (which I could be), the `SourceLine::_path` as an
`std::string*` should be fine since both the `std::string` it points to and the
owner of the `std::string*` will be cleaned up as part of the same teardown of
`AddressLineRange`.
On top of that, the shared_ptr solution looks like it would be much less
elegant as far as I can tell.
I think the set of strings would have to become an
`std::set<std::shared_ptr<std::string>>` and that wouldn't really work because
multiple `shared_ptr` to the same string don't compare as equal.
I just don't see a good, clean solution using `std::shared_ptr`. Maybe I'm
missing something?
Thanks,
Alex
>
> > That way we don't have to do any `nullptr` checks outside of the
> > `SourceLine`
> > implementation. We could just have `SourceLine::path()` return a plain old
> > `std::string` and do something like this:
> >
> > if (_path == nullptr) {
> > return "unknown";
> > else {
> > return *_path;
> > }
>
> A std::shared_ptr has the bool operator to check if the pointer is the nullptr
> so it can store a nullptr. Why not use:
>
> if (!_path) {
> return static_const_unknown_string;
> } else {
> return *(_path.get());
> }
>
> ?
>
> Chris
> _______________________________________________
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
