search

BlackToppStudios / Mezz_Foundation

Foundational Data types that enforce no opinions on how to build games and provide many facilities that complex games need. This largely avoids graphics and physics, but provides tools like the SortedVector, CountedPtr and HashedString classes.
GNU General Public License v3.0
3 stars 0 forks source link

Port-CountedPtr #53

Closed feartheducky closed 5 years ago

feartheducky commented 5 years ago

Port of CountedPtr.h and CountedPtrTests.h.

MakoEnergy commented 5 years ago

In CountedPtr.h: 1) Missing an "e" on line 260 documentation. 2) Use of noexcept is missing from the Reset methods on the CountedPtr. This noexcept may or may not be appropriate, but if it's not then that would be because Aquire and Release wouldn't be noexcept and they currently are. 3) I know I originally was the one that told you to remove the duplicate functions that differed only in the capitalization of the first letter. I was expecting to keep the lower case versions instead of the upper case ones. I'm not sure it actually matters either way and if Sqeaky has a preference I'll go along with that. 4) For the base class of CountedPtrCastImpl on line 562, I see we left the method without an implementation to cause a compiler error (as documented), but I feel it could be cleaner by having an empty class body (no method) or no class body at all (just a declaration). Either of these would communicate the intent to programmers better and would still fail during compilation. The function docs stating the intent would have to be moved to the class docs of course. 5) I won't name line numbers because there are many of them, but do a pass and make sure every sentence in the documentation ends with a period.

MakoEnergy commented 5 years ago

In CountedPtrTests.h: 1) I think the convention for include guards requires the repo name followed by the filename. So instead of "MEZZ_TEST_CountedPtr_h" it should be "MEZZ_FOUNDATION_CountedPtrTests_h". Incidentally we missed the same thing in the SortedVector review. We should go back and fix that while we're at it. >.> 2) I think your vtordisp push and pop only needs to surround classes that virtually inherit. So only needs to be around FooDerived1 and FooDerived2. I don't care enough to actually ask you to change it, but may provide more clarity by only putting it where it needs to be. 3) Very related to 2), if we're using the vtordisp workaround I don't think we need to suppress MSVC 4435 on line 59. 4) Every example class in the CountedPtrTests namespace has an extra level of indent. /pedantic 5) VehicleTest and CarTest ReferenceCountTraits specializations on lines 299 and 311, use typedefs instead of using aliases. /pedantic 6) There are a number of tests where you swapped the ordering of the arguments. For TEST_EQUAL the order is: Name, Expected, Actual. The following are sections where you have them reversed: Automated Destruction, Lines 352-357 Use count and dual handle delete, Lines 385-390 Release and dual handles, Lines 400-413 Dereferencing, Lines 442-450 Reseating pointers and equality, Lines 474-477 and Lines 484-487

Sqeaky commented 5 years ago

On CamelCase vs snake_case and std_case. I think we sort of settled on using CamelCase on just about everything unless we have some compelling in the std library that we are trying to be compatible with.

Like how std::vector:size(), std::map::size() and the other std containers create a pattern of size() method, so our we have an Mezzanine::SortedVector::size method for compatibility with templates or other things that look for size().

Sqeaky commented 5 years ago

List of periods missing in docs that I saw:

Sqeaky commented 5 years ago

This is great! The only thing that I noticed missing was the Benchmark test. We originally added this because C++11 std::shared_ptr wasn't widespread. Now we keep it because share_ptr has a threadsafe reference count which is about 4x as slow as a naked pointer. This skips the thread safe part and that makes it about 2x as slow as naked pointer. This also has the option to use Intrusive Reference counting which brings it to nearly the speed of naked pointers.

In the monolithic repo we have some tests that verify this is faster with a small benchmark ( https://github.com/BlackToppStudios/Mezzanine/blob/master/UnitTests/tests/countedptrtests.h#L657 ). If the std::pointer catches up we want to know, but that is unlikely because this just does less work.

There is an odd preprocessor macro for disabling old tests in there that is clearly not needed anymore. The easiest way to implement it might be to add a fresh test file with a BenchmarkTestGroup and port the benchmark code over.

Sqeaky commented 5 years ago

This looks good. All my comments are nits more than real issues. This clearly works for what is supposed to be.

Here is the output on my machine.

CountedPtr Benchmarks

The objects being created all change a variable on destruction and have initializing, but otherwise trivial constructors. This is useful only for comparing the speeds of the point constructs on this platform, not for providing objective pointer dereferencing costs.

1 - Creating and Dereferencing a raw pointer 10000000 times without reference counting took: 109878472 Microseconds. 2 - Creating and Dereferencing a CountedPtr 10000000 times with internal counting took: 128439308 Microseconds. 3 - Creating and Dereferencing a CountedPtr 10000000 times with external counting took: 206133131 Microseconds. 4 - Creating and Dereferencing a shared_ptr 10000000 times with external counting took: 332991103 Microseconds. 5 - Creating and Dereferencing a shared_ptr from make_shared 10000000 times with external counting took: 228327280 Microseconds. 6 - Creating, Dereferencing and Copying a raw pointer 10000000 times without reference counting took: 103056410 Microseconds. 7 - Creating, Dereferencing and Copying a CountedPtr 10000000 times with internal counting took: 112356380 Microseconds. 8 - Creating, Dereferencing and Copying a CountedPtr 10000000 times with external counting took: 212190584 Microseconds. 9 - Creating, Dereferencing and Copying a shared_ptr 10000000 times with external counting took: 438846639 Microseconds. 10 - Creating, Dereferencing and Copying a shared_ptr from make_shared 10000000 times with external counting took: 336684031 Microseconds.

Checking that raw pointers are fastest for sanity: 1 and 1. Checking CountedPtr internal is faster than CountedPtr external: 1 and 1. Checking CountedPtr internal is faster than shared_ptr: 1 and 1 and 1 and 1. [ Success ] CountedPtrBenchmarks::InternalvsShared [ Success ] CountedPtrBenchmarks::InternalvsSharedCopy [ Success ] CountedPtrBenchmarks::InternalvsMakeShared [ Success ] CountedPtrBenchmarks::InternalvsMakeSharedCopy Checking CountedPtr External is faster than shared_ptr: 1 and 1 and 1 and 1.

                  --= Name: Duration (ns) --- Human Readable =--
             Initial Setup: 564326            0min   0s   0ms 564μs 326ns 
          BitFieldTools-T : 501942            0min   0s   0ms 501μs 942ns 
            CommandLine-T : 819008            0min   0s   0ms 819μs   8ns 
             CountedPtr-T : 853974            0min   0s   0ms 853μs 974ns 
             TupleTools-T : 112147            0min   0s   0ms 112μs 147ns 
              StaticAny-T : 965470            0min   0s   0ms 965μs 470ns 
          StreamLogging-T : 108683            0min   0s   0ms 108μs 683ns 
           BinaryBuffer-T : 1836100           0min   0s   1ms 836μs 100ns 
          Introspection-T : 1474644           0min   0s   1ms 474μs 644ns 
           SortedVector-T : 1822889           0min   0s   1ms 822μs 889ns 
           ManagedArray-T : 3598294           0min   0s   3ms 598μs 294ns 
     SortedManagedArray-T : 3003161           0min   0s   3ms   3μs 161ns 
                FlatMap-T : 43975333          0min   0s  43ms 975μs 333ns 
                 Base64-T : 378737354         0min   0s 378ms 737μs 354ns 
   CountedPtrBenchmarks-S : 2208974501        0min   2s 208ms 974μs 501ns 
       Test Execution Time: 2587844447        0min   2s 587ms 844μs 447ns 
    Summary Reporting Time: 23650             0min   0s   0ms  23μs 650ns

  • Time Spent Reporting Time: 27129 0min 0s 0ms 27μs 129ns 

          ≈ Total Run Time: 2588460171        0min   2s 588ms 460μs 171ns

    --= Summary =-- Success 1418 Warning 0 Skipped 0 Cancelled 0 Inconclusive 0 Failed 0 Unknown 0 NotApplicable 0

    From 1418 tests the worst result is: Success

I am cool with merging this or letting you fix the nits then merging.

Sqeaky commented 5 years ago

Looks Good!