SemiDynamicSparseNumberArray #53
Conversation
|
|
||
| #ifndef METAPHYSICL_DYNAMICSPARSENUMBERBASE_H | ||
| #define METAPHYSICL_DYNAMICSPARSENUMBERBASE_H | ||
| #pragma once |
There was a problem hiding this comment.
Can we switch this back to the usual guard idiom? #pragma once does seem pretty universally supported, but it's not an official part of the standard.
|
This is going to take a while to dig through, but the first thing that screams at me is naming choice. We'll now have a SparseNumberBase that's the base class for DynamicSparseNumberFoo, while at the same time completely unrelated to SparseNumberFoo? Way too misleading. Maybe "Semidynamic" or "Preallocated" or something for the "not dynamic allocation but also not entirely compile-time" option? The name change is also wrecking these diffs. Whatever we end up at for the name, can we please keep the "actual functionality changes" and the "search and replace name change" broken into two separate commits? I'd like to check the former by reading every line, whereas I'd like to ignore the latter by assuming you can use a regexp correctly. :-D |
That's a good name for my planned
There shouldn't actually be any functionality changes in the code movement from |
|
@roystgnr I renamed |
|
Hmm... I guess "Abstract" works better than "Base" as long as we've got pure virtuals there. But wow, should we really have pure virtuals there??? I realize in hindsight that this is all my fault in the first place, for not getting rid of the virtual in safe_bool or at least declaring the derived constructors final, but now that I look at it, it feels completely insane to have a virtual function call in a destructor for a small object intended to be used in temporaries in innermost kernel loops! Could you try your MOOSE AD benchmark with all the destructors marked non-virtual (should be safe, we'll never be deleting from a pointer to base class here, right?) and see if it makes any noticeable difference? |
|
Alright, the newest commit rips out all virtuals (why I had them in the first place was probably because I had some other design in mind), which of course now begs the question of why even have the new class. The only argument I see is to maintain the signature of With the virtuals removed, the Jacobian calculation results in #1 are the fastest yet, although it could just be within error. |
... because they have different behaviors? If DynamicSparseNumberArray is way slower but allows run-time selection of maximum index size, and SemiDynamicSparseNumberArray is way faster but forces you to set and preallocate an upper limit, it seems like you can't get rid of either without losing a valuable option. Am I misunderstanding the question?
Could you post the updated numbers? |
Sorry that was indeed confusing. We definitely want the derived class
But I think we could just get away with:
If we change the template parameters for |
|
Ah, got it. Yeah, the latter seems more reasonable if you can pull it off. Sorry about the hassle (and about setting a bad example with the previous virtuals). |
I never think of it as a hassle to get things right! |
|
Paring of results from #1. Timings are of
|
|
Looks like you're correct on both parts of "fastest yet, although it could just be within error". But if it's not just random noise, going from 13.38 down to 12.47 isn't shabby; let's leave the virtuals off. And of course going from 150 down to 12.47 is huge, as is going from 13.3 to 12.47 while adding flexibility. So I'm definitely liking the outcome here. Do any cleanup you want to do on the branch (I see that fixup commit, and of course getting rid of the 'temporary' renaming would be nice if it's not too impractical) and I'll try to finish reviewing tomorrow. |
|
It was easy to drop the Sigh...I was very fastidious about turning off my formatting save hook, but it looks like not fastidious enough. Especially with being able to just change the template parameters for |
|
Alright, fixed up the diff; looks pretty clean |
|
I was going to complain that this breaks C++98 support, but double-checking master, we broke C++98 support a while ago. You get what you CI test, I guess, huh? |
| namespace MetaPhysicL | ||
| { | ||
| template <typename T, size_t N> | ||
| struct DynamicStdArrayWrapper |
There was a problem hiding this comment.
What's up with DynamicStdArrayWrapper in this file vs in the more name-matching file?
There was a problem hiding this comment.
Lol, I guess I somehow duplicated the file at some point. Will remove
It wouldn't be hard to make this PR C++98 compatible. I know the |
- Uses std::array as storage but tracks size with a member variable - Generalizes DynamicSparseNumberBase in order to create the SemiDynamicSparseNumberArray class template
Really? At first glance it looked like there were a lot of quick fixes (auto -> WhateverSubclassTemplate::iterator, etc), but you're also relying on variadic templates pretty heavily, right? I didn't see any way around that. If you can actually keep all this stuff C++98 compatible that would be great, but that's not a show-stopper; other than the duplicate file you just removed this looked okay to merge as-is. |
You're right. The variadics are definitely a central feature of this PR |
|
We're probably going to change our derivative type in MOOSE to the When do you think MetaPhysicL will be ready for version 1?? |
Honestly? This stuff is great, certainly worth calling it 0.6.0.
I think there's not too much stuff I'd want on my must-have list: fixing up the guards around C++11 and C++14 dependent bits, adding move optimizations to everything ... Optimizations for nested DualNumber (and ideally DualExpression) might be nice, but aren't critical. I think the biggest thing on my wishlist is automated performance testing, though. We really need something like a "make benchmark" that we can just run right off the bat to test proposed changes for improvements or regressions, without anyone having to rebuild (or for other users, download and build!) libMesh and MOOSE first. |
|
An update on this test case: Test specs
SNESComputeJacobian timing
|
|
Linking to idaholab/moose#13963 which also contains some discussion on relative speed of |
DynamicSparseNumberBaseintoDynamicSparseNumberAbstractSemiDynamicSparseNumberArraywhich has the same sparse operations as its pure dynamic counterparts but usesstd::arrayas the backing storage