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.

Sign up for GitHub

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

Apply new frontend design to Container and deriving classes #1115

Merged
merged 7 commits into from
Dec 16, 2021
Merged

Apply new frontend design to Container and deriving classes #1115

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0bed2b0
Make EraseStaleEntries independent template class
franzpoeschel Sep 17, 2021
4a0b172
Container: Split into data and interface class
franzpoeschel Sep 16, 2021
316c9c6
Fix invasive tests
franzpoeschel Sep 30, 2021
2065ec7
Simplify class design for Attributable and Series
franzpoeschel Sep 28, 2021
c497ea3
Apply redesign to Iteration class
franzpoeschel Sep 30, 2021
291deed
Remove LegacyAttributable
franzpoeschel Sep 30, 2021
e23792a
Update documentation
franzpoeschel Oct 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 63 additions & 25 deletions include/openPMD/RecordComponent.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,40 @@ struct IsContiguousContainer< std::array< T_Value, N > >
template< typename T >
class DynamicMemoryView;

class RecordComponent;

namespace internal
{
class RecordComponentData : public BaseRecordComponentData
{
friend class openPMD::RecordComponent;

RecordComponentData();

RecordComponentData( RecordComponentData const & ) = delete;
RecordComponentData( RecordComponentData && ) = delete;

RecordComponentData & operator=( RecordComponentData const & ) = delete;
RecordComponentData & operator=( RecordComponentData && ) = delete;

std::queue< IOTask > m_chunks;
Copy link
Member

Choose a reason for hiding this comment

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

For all the ...Data classes, it would be really good if we try to catch up and add Doxygen strings to the m_ member variables.

Can be a follow-up and we can split the work if we are unsure about some. I remember we found them hard to follow at times and it's good to document the purpose of individual m_ member variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: Inside #1154 I began aliasing these data classes (needed that for something else in there) which made the whole thing a lot more readable in my opinion, e.g.:

namespace internal {
    class RecordComponentData{ /* … */ };
}

class RecordComponent {
    using DataClass = internal::RecordComponentData;
    std::shared_ptr< DataClass > m_recordComponentData;
    /* … */
};

Copy link
Member

@ax3l ax3l Dec 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That is very nice, yes 💖

But my comment was actually about adding doxygen to the members in the data class, so we know what they are for:

struct RecordComponentData
{
    // ...

    //! container for holding data chunks
    std::queue< IOTask > m_chunks;

    //! this sets things on fire
    bool m_fire;
};

I recall that all the original m_dirty and m_flushed members among others were often hard to understand for us without searching through their usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, got you
Yep, these classes are probably a good chance to have documentation for our internal state at one central place

Attribute m_constantValue{ -1 };
/**
* The same std::string that the parent class would pass as parameter to
* RecordComponent::flush().
* This is stored only upon RecordComponent::flush() if
* AbstractIOHandler::flushLevel is set to FlushLevel::SkeletonOnly
* (for use by the Span<T>-based overload of RecordComponent::storeChunk()).
* @todo Merge functionality with ownKeyInParent?
*/
std::string m_name;
bool m_isEmpty = false;
// User has extended the dataset, but the EXTEND task must yet be
// flushed to the backend
bool m_hasBeenExtended = false;
};
}

class RecordComponent : public BaseRecordComponent
{
template<
Expand All @@ -90,10 +124,14 @@ class RecordComponent : public BaseRecordComponent
friend class ParticleSpecies;
template< typename T_elem >
friend class BaseRecord;
template< typename T_elem >
friend class BaseRecordInterface;
friend class Record;
friend class Mesh;
template< typename >
friend class DynamicMemoryView;
friend class internal::RecordComponentData;
friend class MeshRecordComponent;

public:
enum class Allocation
Expand Down Expand Up @@ -248,21 +286,6 @@ class RecordComponent : public BaseRecordComponent

static constexpr char const * const SCALAR = "\vScalar";

virtual ~RecordComponent() = default;

OPENPMD_protected:
RecordComponent();

void readBase();

std::shared_ptr< std::queue< IOTask > > m_chunks;
std::shared_ptr< Attribute > m_constantValue;
std::shared_ptr< bool > m_isEmpty = std::make_shared< bool >( false );
// User has extended the dataset, but the EXTEND task must yet be flushed
// to the backend
std::shared_ptr< bool > m_hasBeenExtended =
std::make_shared< bool >( false );

private:
void flush(std::string const&);
virtual void read();
Expand All @@ -285,19 +308,34 @@ class RecordComponent : public BaseRecordComponent
*/
bool dirtyRecursive() const;

protected:
std::shared_ptr< internal::RecordComponentData > m_recordComponentData{
new internal::RecordComponentData() };

/**
* The same std::string that the parent class would pass as parameter to
* RecordComponent::flush().
* This is stored only upon RecordComponent::flush() if
* AbstractIOHandler::flushLevel is set to FlushLevel::SkeletonOnly
* (for use by the Span<T>-based overload of RecordComponent::storeChunk()).
* @todo Merge functionality with ownKeyInParent?
*/
std::shared_ptr< std::string > m_name = std::make_shared< std::string >();
RecordComponent();

OPENPMD_protected:
RecordComponent( std::shared_ptr< internal::RecordComponentData > );

inline internal::RecordComponentData const & get() const
{
return *m_recordComponentData;
}

inline internal::RecordComponentData & get()
{
return const_cast< internal::RecordComponentData & >(
static_cast< RecordComponent const * >( this )->get() );
}

inline void setData( std::shared_ptr< internal::RecordComponentData > data )
{
m_recordComponentData = std::move( data );
BaseRecordComponent::setData( m_recordComponentData );
}

void readBase();
}; // RecordComponent

} // namespace openPMD

#include "RecordComponent.tpp"
25 changes: 15 additions & 10 deletions include/openPMD/RecordComponent.tpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,10 @@ RecordComponent::makeConstant(T value)
if( written() )
throw std::runtime_error("A recordComponent can not (yet) be made constant after it has been written.");

*m_constantValue = Attribute(value);
*m_isConstant = true;
auto & rc = get();

rc.m_constantValue = Attribute(value);
rc.m_isConstant = true;
return *this;
}

Expand Down Expand Up @@ -133,13 +135,14 @@ inline void RecordComponent::loadChunk(
if( !data )
throw std::runtime_error("Unallocated pointer passed during chunk loading.");

auto & rc = get();
if( constant() )
{
uint64_t numPoints = 1u;
for( auto const& dimensionSize : extent )
numPoints *= dimensionSize;

T value = m_constantValue->get< T >();
T value = rc.m_constantValue.get< T >();

T* raw_ptr = data.get();
std::fill(raw_ptr, raw_ptr + numPoints, value);
Expand All @@ -150,7 +153,7 @@ inline void RecordComponent::loadChunk(
dRead.extent = extent;
dRead.dtype = getDatatype();
dRead.data = std::static_pointer_cast< void >(data);
m_chunks->push(IOTask(this, dRead));
rc.m_chunks.push(IOTask(this, dRead));
}
}

Expand Down Expand Up @@ -201,7 +204,8 @@ RecordComponent::storeChunk(std::shared_ptr<T> data, Offset o, Extent e)
dWrite.dtype = dtype;
/* std::static_pointer_cast correctly reference-counts the pointer */
dWrite.data = std::static_pointer_cast< void const >(data);
m_chunks->push(IOTask(this, dWrite));
auto & rc = get();
rc.m_chunks.push(IOTask(this, dWrite));
}

template< typename T_ContiguousContainer >
Expand Down Expand Up @@ -289,14 +293,15 @@ RecordComponent::storeChunk( Offset o, Extent e, F && createBuffer )
*/
if( !written() )
{
auto & rc = get();
Parameter< Operation::CREATE_DATASET > dCreate;
dCreate.name = *m_name;
dCreate.name = rc.m_name;
dCreate.extent = getExtent();
dCreate.dtype = getDatatype();
dCreate.chunkSize = m_dataset->chunkSize;
dCreate.compression = m_dataset->compression;
dCreate.transform = m_dataset->transform;
dCreate.options = m_dataset->options;
dCreate.chunkSize = rc.m_dataset.chunkSize;
dCreate.compression = rc.m_dataset.compression;
dCreate.transform = rc.m_dataset.transform;
dCreate.options = rc.m_dataset.options;
IOHandler()->enqueue(IOTask(this, dCreate));
}
Parameter< Operation::GET_BUFFER_VIEW > getBufferView;
Expand Down
13 changes: 12 additions & 1 deletion include/openPMD/backend/Attributable.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ attr_value_check( std::string const key, std::string const value )
"' must not be empty!" );
}

template< typename > class BaseRecordData;
} // namespace internal

/** @brief Layer to manage storage of attributes associated with file objects.
Expand All @@ -119,6 +120,10 @@ class AttributableInterface
friend Writable* getWritable(AttributableInterface*);
template< typename T_elem >
friend class BaseRecord;
template< typename T_elem >
friend class BaseRecordInterface;
template< typename >
friend class internal::BaseRecordData;
template<
typename T,
typename T_key,
Expand Down Expand Up @@ -351,6 +356,12 @@ class AttributableInterface
return m_attri->m_writable;
}

inline
void setData( internal::AttributableData * attri )
{
m_attri = attri;
}

inline
internal::AttributableData & get()
{
Expand Down Expand Up @@ -407,7 +418,7 @@ class LegacyAttributable : public AttributableInterface
public:
LegacyAttributable() : AttributableInterface{ nullptr }
{
AttributableInterface::m_attri = m_attributableData.get();
AttributableInterface::setData( m_attributableData.get() );
}
};

Expand Down
Loading