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.
> 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
