Skip to content

Commit

Permalink
Problem: Reading a real-world DICOM image was a bit slow, and simple …
Browse files Browse the repository at this point in the history 
…profiling suggested that some 50% of the time (*) was spent in `DcmList::seek_to` (which has a random-access-like interface, but internally _iterates_ over a doubly linked list)

Solution: Internally use `std::deque` that provides fast O(1) random access

(*): the other 50% was spent decoding jpeg tiles, which is legit use of time of course
  • Loading branch information
@reunanen
reunanen committed Jan 3, 2025
1 parent ae63526 commit 738ff84
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 208 deletions.
66 changes: 9 additions & 57 deletions dcmdata/include/dcmtk/dcmdata/dclist.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,53 +22,13 @@
#ifndef DCLIST_H
#define DCLIST_H

#include "dcmtk/config/osconfig.h" /* make sure OS specific configuration is included first */

#include "dcmtk/ofstd/ofcast.h"
#include "dcmtk/ofstd/oftypes.h"

#include "dcmtk/dcmdata/dcobject.h"

#include <deque>

/// index indicating "end of list"
const unsigned long DCM_EndOfListIndex = OFstatic_cast(unsigned long, -1L);

/** helper class maintaining an entry in a DcmList double-linked list
*/
class DCMTK_DCMDATA_EXPORT DcmListNode
{

public:
/** constructor
* @param obj object to be maintained by this list node
*/
DcmListNode( DcmObject *obj );

/// destructor
~DcmListNode();

/// return pointer to object maintained by this list node
inline DcmObject *value() { return objNodeValue; }

private:
friend class DcmList;

/// pointer to next node in double-linked list
DcmListNode *nextNode;

/// pointer to previous node in double-linked list
DcmListNode *prevNode;

/// pointer to DcmObject instance maintained by this list entry
DcmObject *objNodeValue;

/// private undefined copy constructor
DcmListNode(const DcmListNode &);

/// private undefined copy assignment operator
DcmListNode &operator=(const DcmListNode &);

};

/// list position indicator
typedef enum
{
Expand Down Expand Up @@ -101,13 +61,13 @@ class DCMTK_DCMDATA_EXPORT DcmList
/// destructor
~DcmList();

/** insert object at end of list
/** insert object at end of list and make it be the "current" one
* @param obj pointer to object
* @return pointer to object
*/
DcmObject *append( DcmObject *obj );

/** insert object at start of list
/** insert object at start of list and make it be the "current" one
* @param obj pointer to object
* @return pointer to object
*/
Expand Down Expand Up @@ -153,27 +113,19 @@ class DCMTK_DCMDATA_EXPORT DcmList
void deleteAllElements();

/// return cardinality of list
inline unsigned long card() const { return cardinality; }
inline unsigned long card() const { return static_cast<unsigned long>(objects.size()); }

/// return true if list is empty, false otherwise
inline OFBool empty(void) const { return firstNode == NULL; }
inline OFBool empty(void) const { return objects.empty(); }

/// return true if current node exists, false otherwise
inline OFBool valid(void) const { return currentNode != NULL; }
inline OFBool valid(void) const { return current != objects.end(); }

private:
/// pointer to first node in list
DcmListNode *firstNode;

/// pointer to last node in list
DcmListNode *lastNode;
std::deque<DcmObject*> objects;

/// pointer to current node in list
DcmListNode *currentNode;
std::deque<DcmObject*>::iterator current;

/// number of elements in list
unsigned long cardinality;

/// private undefined copy constructor
DcmList &operator=(const DcmList &);

Expand Down
180 changes: 29 additions & 151 deletions dcmdata/libsrc/dclist.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,43 +19,16 @@
*
*/

#include "dcmtk/config/osconfig.h" /* make sure OS specific configuration is included first */

#include "dcmtk/ofstd/ofstream.h"
#include "dcmtk/dcmdata/dclist.h"


// *****************************************
// *** DcmListNode *************************
// *****************************************


DcmListNode::DcmListNode( DcmObject *obj )
: nextNode(NULL),
prevNode(NULL),
objNodeValue(obj)
{
}


// ********************************


DcmListNode::~DcmListNode()
{
}


// *****************************************
// *** DcmList *****************************
// *****************************************


DcmList::DcmList()
: firstNode(NULL),
lastNode(NULL),
currentNode(NULL),
cardinality(0)
: current(objects.end())
{
}

Expand All @@ -65,17 +38,7 @@ DcmList::DcmList()

DcmList::~DcmList()
{
if ( !DcmList::empty() ) // list is not empty !
{
lastNode->nextNode = NULL; // set to 0 for safety reasons
do {
DcmListNode *temp = firstNode;
firstNode = firstNode->nextNode;
// delete temp->objNodeValue; // dangerous!
delete temp;
} while ( firstNode != NULL );
currentNode = firstNode = lastNode = NULL;
}
deleteAllElements();
}


Expand All @@ -84,19 +47,7 @@ DcmList::~DcmList()

DcmObject *DcmList::append( DcmObject *obj )
{
if ( obj != NULL )
{
if ( DcmList::empty() ) // list is empty !
currentNode = firstNode = lastNode = new DcmListNode(obj);
else
{
DcmListNode *node = new DcmListNode(obj);
lastNode->nextNode = node;
node->prevNode = lastNode;
currentNode = lastNode = node;
}
cardinality++;
} // obj == NULL
current = objects.insert(objects.end(), obj);
return obj;
}

Expand All @@ -106,19 +57,7 @@ DcmObject *DcmList::append( DcmObject *obj )

DcmObject *DcmList::prepend( DcmObject *obj )
{
if ( obj != NULL )
{
if ( DcmList::empty() ) // list is empty !
currentNode = firstNode = lastNode = new DcmListNode(obj);
else
{
DcmListNode *node = new DcmListNode(obj);
node->nextNode = firstNode;
firstNode->prevNode = node;
currentNode = firstNode = node;
}
cardinality++;
} // obj == NULL
current = objects.insert(objects.begin(), obj);
return obj;
}

Expand All @@ -132,8 +71,7 @@ DcmObject *DcmList::insert( DcmObject *obj, E_ListPos pos )
{
if ( DcmList::empty() ) // list is empty !
{
currentNode = firstNode = lastNode = new DcmListNode(obj);
cardinality++;
DcmList::append(obj);
}
else {
if ( pos==ELP_last )
Expand All @@ -146,30 +84,12 @@ DcmObject *DcmList::insert( DcmObject *obj, E_ListPos pos )
DcmList::append( obj ); // cardinality++;
else if ( pos == ELP_prev ) // insert before current node
{
DcmListNode *node = new DcmListNode(obj);
if ( currentNode->prevNode == NULL )
firstNode = node; // insert at the beginning
else
currentNode->prevNode->nextNode = node;
node->prevNode = currentNode->prevNode;
node->nextNode = currentNode;
currentNode->prevNode = node;
currentNode = node;
cardinality++;
current = objects.insert(current, obj);
}
else //( pos==ELP_next || pos==ELP_atpos )
// insert after current node
{
DcmListNode *node = new DcmListNode(obj);
if ( currentNode->nextNode == NULL )
lastNode = node; // append to the end
else
currentNode->nextNode->prevNode = node;
node->nextNode = currentNode->nextNode;
node->prevNode = currentNode;
currentNode->nextNode = node;
currentNode = node;
cardinality++;
current = objects.insert(std::next(current), obj);
}
}
} // obj == NULL
Expand All @@ -183,30 +103,15 @@ DcmObject *DcmList::insert( DcmObject *obj, E_ListPos pos )
DcmObject *DcmList::remove()
{
DcmObject *tempobj;
DcmListNode *tempnode;

if ( DcmList::empty() ) // list is empty !
return NULL;
else if ( !DcmList::valid() )
return NULL; // current node is 0
else
{
tempnode = currentNode;

if ( currentNode->prevNode == NULL )
firstNode = currentNode->nextNode; // delete first element
else
currentNode->prevNode->nextNode = currentNode->nextNode;

if ( currentNode->nextNode == NULL )
lastNode = currentNode->prevNode; // delete last element
else
currentNode->nextNode->prevNode = currentNode->prevNode;

currentNode = currentNode->nextNode;
tempobj = tempnode->value();
delete tempnode;
cardinality--;
tempobj = *current;
current = objects.erase(current);
return tempobj;
}
}
Expand All @@ -229,23 +134,29 @@ DcmObject *DcmList::seek( E_ListPos pos )
switch (pos)
{
case ELP_first :
currentNode = firstNode;
current = objects.begin();
break;
case ELP_last :
currentNode = lastNode;
if (objects.empty())
current = objects.end();
else
current = std::prev(objects.end());
break;
case ELP_prev :
if ( DcmList::valid() )
currentNode = currentNode->prevNode;
if (current == objects.begin())
current = objects.end();
else
--current;
break;
case ELP_next :
if ( DcmList::valid() )
currentNode = currentNode->nextNode;
++current;
break;
default:
break;
}
return DcmList::valid() ? currentNode->value() : NULL;
return DcmList::valid() ? *current : NULL;
}


Expand All @@ -254,23 +165,11 @@ DcmObject *DcmList::seek( E_ListPos pos )

DcmObject *DcmList::seek_to(unsigned long absolute_position)
{
if (absolute_position < cardinality / 2)
{
/* iterate over first half of the list */
seek( ELP_first );
for (unsigned long i = 0; i < absolute_position; i++)
seek( ELP_next );
}
else if (absolute_position < cardinality)
{
/* iterate over second half of the list (starting from the end) */
seek( ELP_last );
for (unsigned long i = absolute_position + 1; i < cardinality; i++)
seek( ELP_prev );
} else {
/* invalid position */
currentNode = NULL;
}
if (absolute_position < objects.size())
current = objects.begin() + absolute_position;
else
current = objects.end();

return get( ELP_atpos );
}

Expand All @@ -280,29 +179,8 @@ DcmObject *DcmList::seek_to(unsigned long absolute_position)

void DcmList::deleteAllElements()
{
unsigned long numElements = cardinality;
DcmListNode* tmpNode = NULL;
DcmObject* tmpObject = NULL;
// delete all elements
for (unsigned long i = 0; i < numElements; i++)
{
// always select first node so no search is necessary
tmpNode = firstNode;
// clear value of node
tmpObject = tmpNode->value();
if (tmpObject != NULL)
{
// delete load of selected list node
delete tmpObject;
tmpObject = NULL;
}
firstNode = tmpNode->nextNode;
// delete the list node itself
delete tmpNode;
}
// reset all attributes for later use
firstNode = NULL;
lastNode = NULL;
currentNode = NULL;
cardinality = 0;
for (DcmObject* object : objects)
delete object;
objects.clear();
current = objects.end();
}

0 comments on commit 738ff84

Please sign in to comment.