Fix/Logger memory leak

[SNiukalov]

Fixes #5239

Jira task

PR in smartdevicelink

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

ATF scripts

Summary

Updated log4cxx library to avoid leak and accumulation memory.

Bug Fixes

• log4cxx memory accumulation

CLA

• I have signed the CLA

[ydementieiev]

@SNiukalov maybe after you renamed primary -> primary_ it's time to rename primary1 -> primary? or primary1 have some special meaning. And the same below with backup1

[ydementieiev]

@SNiukalov why is name1?

[AGaliuzov]

Guys. Could someone explain how the weak_ptr usage allows to avoid memory leaks?

I would say that this is crucial to use raw pointer (or better reference here) due to performance matter. Logging the stuff is time consuming operation. When you create the shared pointer to log the stuff out during every attempt to log what will happen with performance?
Every attempt to log will do what? Create shared pointer, perform atomic incrementing of counter and so on. Is it what we re expecting here?

[SNiukalov]

@AGaliuzov
Hi, yes. Using weak_ptr avoids cross-references due to which memory leak occurs
small example:

struct B;
struct A {
std::shared_ptr b;
~A() { std::cout << "~A()\n"; }
};
struct B {
std::shared_ptr a;
~B() { std::cout << "~B()\n"; }
};
void useAnB() {
auto a = std::make_shared();
auto b = std::make_shared();
a->b = b;
b->a = a;
}
int main() {
useAnB();
std::cout << "Finished using A and B\n";
}

In this case, destructors for A and B will not be called.
If in structure A to replace the shared by weak all destructors will be called

[AGaliuzov]

@AGaliuzov
Hi, yes. Using weak_ptr avoids cross-references due to which memory leak occurs
small example:

struct B;
struct A {
std::shared_ptr b;
~A() { std::cout << "~A()\n"; }
};
struct B {
std::shared_ptr a;
~B() { std::cout << "~B()\n"; }
};
void useAnB() {
auto a = std::make_shared();
auto b = std::make_shared();
a->b = b;
b->a = a;
}
int main() {
useAnB();
std::cout << "Finished using A and B\n";
}

In this case, destructors for A and B will not be called.
If in structure A to replace the shared by weak all destructors will be called

I know how the stuff works in general. Here I believe you try to cover the real problem bu substituting shared to weak.

Idea of the logger is to have the only logger instance. Could you show SDL code where we have such a cross reference? May be to worth to fix exactly this place.?

[SNiukalov]

@AGaliuzov
In this case, we fixed the memory accumulation in the log4cxx library.
And a memory leak according to https://issues.apache.org/jira/browse/LOGCXX-486
weak_ptr in this case does not incur any additional costs and was proposed as a small optimization.
Memory leak from SDL will be fix in other pull request

[AGaliuzov]

@AGaliuzov
In this case, we fixed the memory accumulation in the log4cxx library.
And a memory leak according to https://issues.apache.org/jira/browse/LOGCXX-486
weak_ptr in this case does not incur any additional costs and was proposed as a small optimization.
Memory leak from SDL will be fix in other pull request

I wouldn't say weak_ptr is optimisation. Still you need to make it shared all the time. To me the weak_ptr approach here not needed. But let other guys to tel their opinion.

@AByzhynar , @LuxoftAKutsan ?

[mvorobio]

According to diagrams seems the pool fix does the trick even without extra efforts related to pointers replacement. Perhaps, it is worth to integrate this fix only?

[LuxoftAKutsan]

@SNiukalov could you please attach diagrams of memory usage with applying each commit

[LuxoftAKutsan]

@AGaliuzov using shared instead of weak pointers may cause memory leaks due to cross-references. Weak pointers may fix the issue.
I don't really know if d663969 do everything well. it is to big for deep analisys.
Agree that using references of even raw pointers would do a better job because of performance issues of shared\weak pointers.
But we can't base solutions to apply such an approach without measurment\profiling.

Our goal, for now, is to avoid critical memory leaks inside the logger.

I believe that our version of log4cxx is critically outdated and the best solution would be to implement https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0046-implement-logger-abstraction.md and apply another more robust logger with better performance and support. Supporting of old log4cxx version is not our goal, it requires many resources, so we can fix for now critical issues, and try to push the correct solution.

In this PR I can see several solutions that are combined,
For now, I would like so see one more time memory usage graphs with applying each of this solution and in case if d663969 does not prevent memory leaks, I agree that we do not need it.
So @SNiukalov please attach graphs with descriptions what commits are applied

[AGaliuzov]

@LuxoftAKutsan

may cause memory leaks

May cause and cause with proof - are different things. Don't you agree?

You could even say - using shared pointer causes memory leaks. Or even more - allocating the memory may cause memory leak.

Nut there is one big if - it causes leak if you don't free the memory. It causes leak if you are really have cross reference. Do you?

[LuxoftAKutsan]

I can see fix_pool + weak_ptr make graph more flat. Not clear if shared ptr allied in this graphs.

[SNiukalov]

@LuxoftAKutsan @AGaliuzov @AByzhynar @mvorobio
I will investigate this problem more deeply, because I think this problem is the porting of previous SDL changes to the smart_pointers branch and after that attaches the memory usage diagrams with each commit.

[mvorobio]

@LuxoftAKutsan

I can see fix_pool + weak_ptr make graph more flat.

Please pay attention to different scales on the diagrams above. It is difficult to make such a conclusion visually.

[SNiukalov]

@LuxoftAKutsan @AGaliuzov @AByzhynar @mvorobio

[SNiukalov]

@LuxoftAKutsan @AGaliuzov @AByzhynar @mvorobio
Relation to the above proposed report, I consider it necessary to apply the switch to the branch smart_pointers + fix pool and to remove the changes due to weak_ptr since they are not completed.

Guys, please, to tell your opinion.

[mvorobio]

From my point all options with the pool fix give almost identical results regarding memory usage.

[SNiukalov]

@LuxoftAKutsan @AGaliuzov @AByzhynar
Guys, please, to tell your opinion.

[alexkutsan]

smart_pointers + fix pool and to remove the changes due to weak_ptr since they are not completed.
@SNiukalov agree with proposition

[LitvinenkoIra]

@LuxoftAKutsan @AByzhynar @mvorobio please check the latest updates

[LuxoftAKutsan]

@SNiukalov looks like we have complete internal review. You are free to create review in sdl_core.