Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Any type with configurable small buffer optimization #1695

Merged
merged 21 commits into from
Dec 7, 2022

Conversation

paulgessinger
Copy link
Member

No description provided.

@paulgessinger paulgessinger added this to the next milestone Nov 24, 2022
@codecov
Copy link

codecov bot commented Nov 24, 2022

Codecov Report

Merging #1695 (ede16ad) into main (3dfba63) will increase coverage by 0.20%.
The diff coverage is 73.48%.

@@            Coverage Diff             @@
##             main    #1695      +/-   ##
==========================================
+ Coverage   49.07%   49.28%   +0.20%     
==========================================
  Files         397      398       +1     
  Lines       21606    21787     +181     
  Branches     9829     9884      +55     
==========================================
+ Hits        10604    10737     +133     
- Misses       4188     4201      +13     
- Partials     6814     6849      +35     
Impacted Files Coverage Δ
Core/include/Acts/Utilities/Any.hpp 73.48% <73.48%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions
Copy link

github-actions bot commented Nov 24, 2022

📊 Physics performance monitoring for ede16ad

Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: seeded, truth smeared, truth estimated, orthogonal
Ambiguity resolution: seeded, orthogonal
Truth tracking
Truth tracking (GSF)

Vertexing

Vertexing vs. mu
IVF seeded

IVF truth_smeared

IVF truth_estimated

IVF orthogonal

Seeding

Seeding seeded

Seeding truth_estimated

Seeding orthogonal

CKF

CKF seeded

CKF truth_smeared

CKF truth_estimated

CKF orthogonal

Ambiguity resolution

seeded

Truth tracking (Kalman Filter)

Truth tracking

Truth tracking (GSF)

Truth tracking

@paulgessinger
Copy link
Member Author

I ran valgrind on the unit tests which indicates I'm not leaking memory:

$ valgrind --tool=memcheck bin/ActsUnitTestAny
==3104117== Memcheck, a memory error detector
==3104117== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==3104117== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==3104117== Command: bin/ActsUnitTestAny
==3104117==
Running 8 test cases...

*** No errors detected
==3104117==
==3104117== HEAP SUMMARY:
==3104117==     in use at exit: 0 bytes in 0 blocks
==3104117==   total heap usage: 13,141 allocs, 13,141 frees, 5,494,543 bytes allocated
==3104117==
==3104117== All heap blocks were freed -- no leaks are possible
==3104117==
==3104117== For lists of detected and suppressed errors, rerun with: -s
==3104117== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

For some reason, heaptrack thinks there is a memory leak, however:

$ heaptrack bin/ActsUnitTestAny
heaptrack output will be written to "/home/pagessin/dev/acts/build/heaptrack.ActsUnitTestAny.3104211.gz"
starting application, this might take some time...
Running 8 test cases...

*** No errors detected
free(): invalid pointer
Aborted (core dumped)
heaptrack stats:
        allocations:            13141
        leaked allocations:     376
        temporary allocations:  10044
Heaptrack finished! Now run the following to investigate the data:

Copy link
Member

@benjaminhuth benjaminhuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite impressive work I would say! Just some minor comments, nothing critical...

Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@HadrienG2 HadrienG2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through this and it's not immediately obvious to me why an invalid free is observed in the heaptrack case, but have a few comments anyway.

Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
@HadrienG2
Copy link
Contributor

HadrienG2 commented Nov 25, 2022

For context, I spotted what's going on in the heaptrack test above. Notice how the test output changes from...

Running 8 test cases...

*** No errors detected

...in the valgrind case to...

Running 8 test cases...

*** No errors detected
free(): invalid pointer
Aborted (core dumped)

...in the heaptrack case. What this means is that the memory allocator is bombing on us in the heaptrack case, hinting at some possible misuse of it. And obviously, an app that crashes does not free all of its allocations...

Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
@paulgessinger
Copy link
Member Author

Ok I think I addressed all comments. heaptrack is still unhappy, valgrind is.
I added optional intrusive tracking of heap allocations and deallocations, which also indicates there's no heap memory leak.

With this caveat, should we move forward with this?
If everyone is happy, I'll put in doxygen comments.

Copy link
Contributor

@HadrienG2 HadrienG2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is now in a pretty good shape. Please consider this last round of suggestions, and then ping me back once the doxygen is done for final approval.

Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/Any.hpp Outdated Show resolved Hide resolved
@paulgessinger paulgessinger changed the title feat: Any type with small buffer optimization feat: Any type with configurable small buffer optimization Nov 25, 2022
@paulgessinger
Copy link
Member Author

When I run heaptrack on a standalone executable, it reports this free error, even when I remove the Any part completely. I suspect more and more that this error doesn't have anything to do with the implementation here.

@HadrienG2
Copy link
Contributor

My previous advice to throw a debugger at this still stands :)

@paulgessinger paulgessinger removed the 🚧 WIP Work-in-progress label Dec 2, 2022
@paulgessinger
Copy link
Member Author

I ran with a debugger, the stack trace looks to me like the abort comes from inside heaptrack code. There's a non-zero chance that this is because of some memory corruption, but the fact that if I run heaptrack on any binary I get the same abort, and also valgrind is happy gives me some confidence that there is no memory corruption happening.

@kodiakhq kodiakhq bot merged commit d5ec42e into acts-project:main Dec 7, 2022
@github-actions github-actions bot removed the automerge label Dec 7, 2022
@paulgessinger paulgessinger modified the milestones: next, v22.0.0 Dec 21, 2022
CarloVarni pushed a commit to CarloVarni/acts that referenced this pull request Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants