Scope file handles to BundleResource and BundleResourceStream life-times, resolves #309

[achristoforides]

This PR introduces changes which resolve issue #309 .

Instead of closing BundleResource file handles at the end of the Bundle life-time, they are now cleaned up if there are no opened resources in a given bundle/bundle container. To achieve this, a reference counting method is used to keep track of how many opened resources there are associated with the given bundle. If this number reaches 0, then the container is closed (thus closing the file handles). If a user requests a resource after the bundle has been closed, it is then reopened and their resource is then served all while keeping track of the reference count.

[jeffdiclemente]

Tests need to be added to framework\test\gtest\OpenFileHandleTest.cpp for this new behavior.

[jeffdiclemente]

@saschazelzer do you have any flags for this? If not i'm planning on merging this in on Monday. Thanks.

[jeffdiclemente]

I looked over the code one more time and noticed a couple things.

[jeffdiclemente]

The code coverage failure can be solved with a slightly different solution. If the default member initialization of numOpenResources is moved to the constructor and the friend classes are removed, at least locally on my machine, this fixes the code coverage error.

I still don't know why this solves the issue though.

My next step is to isolate the friend class change and see if that causes the same code coverage failure.

[jeffdiclemente]

The code coverage failure can be solved with a slightly different solution. If the default member initialization of numOpenResources is moved to the constructor and the friend classes are removed, at least locally on my machine, this fixes the code coverage error.

I still don't know why this solves the issue though.

My next step is to isolate the friend class change and see if that causes the same code coverage failure.

adding back the friend class (and not the default member initialization of numOpenResources) does not produce the same gcov file containing ~20 lines containing /*EOF*/ (which ultimately leads to cmake failing).

[jeffdiclemente]

performance should be measured within the google benchmark tests to see what the overhead is now that a mutex has been introduced.

[jeffdiclemente]

performance should be measured within the google benchmark tests to see what the overhead is now that a mutex has been introduced.

I ran the two bundle install benchmarks against development and this branch. Here are the results:

Benchmark Test	development Time	development CPU	achristoforides:309-scope-file-handle Time	achristoforides:309-scope-file-handle CPU
BundleInstallFixture/BundleInstallCppFramework/manual_time_mean	274950 ns	331298 ns	284231 ns	342625 ns
BundleInstallFixture/BundleInstallCppFramework/manual_time_median	273459 ns	328281 ns	282698 ns	339929 ns
BundleInstallFixture/BundleInstallCppFramework/manual_time_stddev	17049 ns	19412 ns	17422 ns	19760 ns
BundleInstallFixture/LargeBundleInstallCppFramework/manual_time_mean	271779 ns	336457 ns	276112 ns	333663 ns
BundleInstallFixture/LargeBundleInstallCppFramework/manual_time_median	269830 ns	335545 ns	274588 ns	331362 ns
BundleInstallFixture/LargeBundleInstallCppFramework/manual_time_stddev	16674 ns	19432 ns	15395 ns	17959 ns

[jeffdiclemente]

I'd like to hold off on merging this for now. This change does regress bundle install performance which I believe is important.

[achristoforides]

Benchmark Test	development Time	development CPU	achristoforides:309-scope-file-handle Time	achristoforides:309-scope-file-handle CPU
BundleInstallFixture/BundleInstallCppFramework/manual_time_mean	157288 ns	173804 ns	152611 ns	168636 ns
BundleInstallFixture/BundleInstallCppFramework/manual_time_median	156323 ns	172772 ns	152249 ns	168275 ns
BundleInstallFixture/BundleInstallCppFramework/manual_time_stddev	6680 ns	7311 ns	1261 ns	1404 ns
BundleInstallFixture/LargeBundleInstallCppFramework/manual_time_mean	156747 ns	173751 ns	154907 ns	171052 ns
BundleInstallFixture/LargeBundleInstallCppFramework/manual_time_median	156204 ns	173087 ns	154609 ns	170763 ns
BundleInstallFixture/LargeBundleInstallCppFramework/manual_time_stddev	1865 ns	2146 ns	1116 ns	1262 ns

I updated this PR by removing the mutex in BundleArchive and replacing it with a std::atomic implementation. The results in this table were generated by running the benchmark test (in Release mode) with --benchmark_repetitions=256. As you can see, the timings are actually better compared to what they were before this change.

Update: I've re-ran the tests and updated the table; the other tests were ran while a few applications were running on my computer. I understand that running these benchmark tests in a non-supercomputer like environment can result in varying results, but these are the results I got.

[achristoforides]

Benchmark Test	development Time	development CPU	achristoforides:309-scope-file-handle Time	achristoforides:309-scope-file-handle CPU
BundleInstallFixture/BundleInstallCppFramework/manual_time_mean	395675 ns	450865 ns	399481 ns	451873 ns
BundleInstallFixture/BundleInstallCppFramework/manual_time_median	374271 ns	439257 ns	371490 ns	442217 ns
BundleInstallFixture/BundleInstallCppFramework/manual_time_stddev	46192 ns	56893 ns	50370 ns	64054 ns
BundleInstallFixture/LargeBundleInstallCppFramework/manual_time_mean	388965 ns	454660 ns	402515 ns	453679 ns
BundleInstallFixture/LargeBundleInstallCppFramework/manual_time_median	367462 ns	443154 ns	378240 ns	442913 ns
BundleInstallFixture/LargeBundleInstallCppFramework/manual_time_stddev	47679 ns	59251 ns	51271 ns	66591 ns

Newly ran test (release mode, 512 iterations) comparing the mutex implementation vs. the development branch.