Richard Bown wrote:
> Looks like to me this is because of the Event construction taking
> place in each one [...]
>
> I've just created this scratchy DummyEvent that you can reset the
> times for and substituted it into Composition, Segment,
> SegmentNotationHelper to see if it helps.
I think you're going about this the wrong way. Your DummyEvent could
be useful to help determine whether Event construction really is the
overhead, but it's rather a roundabout way of doing that and you
don't seem to have actually done any measurements to support it anyway.
(What I mean is, once cachegrind or whatever suggests that a particular
function may be slow, the obvious thing to do is start timing that
function in isolation against alternatives, not just reimplement
something and make vague noises about how it seems better.)
And besides, if it's determined that Event construction is too slow,
surely the right thing to do is try to optimise that?
So... I tried putting another three-line loop into base/test.C to
time how long construction actually takes:
Event: 200000 event ctors alone: 8710ms
Event: 200000 event ctors plus getAbsTime/Duration/SubOrdering: 8150ms
Event: 200000 event ctors plus one get/set and
getAbsTime/Duration/SubOrdering: 8510ms
You're right, that is pretty slow. And it's probably because the
property map (m_data.m_properties) is constructed every time, even
if nothing's going to go into it. (I thought about that when writing
the class but I didn't think propertyless Events were going to be
very common -- I was forgetting about cases like this.)
So, let's make m_data.m_properties into a pointer and construct it
on demand. (Patch attached -- please test that this doesn't seem to
break anything for you before I commit it.) Timings now:
Event: 200000 event ctors alone: 230ms
Event: 200000 event ctors plus getAbsTime/Duration/SubOrdering: 230ms
Event: 200000 event ctors plus one get/set and
getAbsTime/Duration/SubOrdering: 2330ms
Much better, and it doesn't seem to have had any adverse effect on
the other timings in test.C. OK, so this is still going to be a bit
slower than just setting a couple of variables, but 1.15usec per call
is unlikely to be much of a bottleneck any more, and this may help
elsewhere too, and it's got to be better than introducing a nasty
specialised class just for optimisation purposes.
Chris
Index: base/Event.C
===================================================================
RCS file: /cvsroot/rosegarden/base/Event.C,v
retrieving revision 1.42
diff -u -p -c -r1.42 Event.C
*** base/Event.C 22 Apr 2003 15:14:30 -0000 1.42
--- base/Event.C 2 Jun 2003 09:51:13 -0000
*************** Event::EventData::EventData(const std::s
*** 47,66 ****
m_type(type),
m_absoluteTime(absoluteTime),
m_duration(duration),
! m_subOrdering(subOrdering)
{
// empty
}
Event::EventData::EventData(const std::string &type, timeT absoluteTime,
timeT duration, short subOrdering,
! const PropertyMap &properties) :
m_refCount(1),
m_type(type),
m_absoluteTime(absoluteTime),
m_duration(duration),
m_subOrdering(subOrdering),
! m_properties(properties)
{
// empty
}
--- 47,67 ----
m_type(type),
m_absoluteTime(absoluteTime),
m_duration(duration),
! m_subOrdering(subOrdering),
! m_properties(0)
{
// empty
}
Event::EventData::EventData(const std::string &type, timeT absoluteTime,
timeT duration, short subOrdering,
! const PropertyMap *properties) :
m_refCount(1),
m_type(type),
m_absoluteTime(absoluteTime),
m_duration(duration),
m_subOrdering(subOrdering),
! m_properties(properties ? new PropertyMap(*properties) : 0)
{
// empty
}
*************** Event::EventData::~EventData()
*** 83,125 ****
timeT
Event::EventData::getNotationTime() const
{
! PropertyMap::const_iterator i = m_properties.find(NotationTime);
! if (i == m_properties.end()) return m_absoluteTime;
else return static_cast<PropertyStore<Int> *>(i->second)->getData();
}
timeT
Event::EventData::getNotationDuration() const
{
! PropertyMap::const_iterator i = m_properties.find(NotationDuration);
! if (i == m_properties.end()) return m_duration;
else return static_cast<PropertyStore<Int> *>(i->second)->getData();
}
void
Event::EventData::setTime(const PropertyName &name, timeT t, timeT deft)
{
! PropertyMap::iterator i = m_properties.find(name);
if (t != deft) {
! if (i == m_properties.end()) {
! m_properties.insert(PropertyPair(name, new PropertyStore<Int>(t)));
} else {
static_cast<PropertyStore<Int> *>(i->second)->setData(t);
}
! } else if (i != m_properties.end()) {
delete i->second;
! m_properties.erase(i);
}
}
PropertyMap *
Event::find(const PropertyName &name, PropertyMap::iterator &i)
{
! PropertyMap *map = &m_data->m_properties;
! i = map->find(name);
! if (i == map->end()) {
map = m_nonPersistentProperties;
if (!map) return 0;
--- 84,128 ----
timeT
Event::EventData::getNotationTime() const
{
! if (!m_properties) return m_absoluteTime;
! PropertyMap::const_iterator i = m_properties->find(NotationTime);
! if (i == m_properties->end()) return m_absoluteTime;
else return static_cast<PropertyStore<Int> *>(i->second)->getData();
}
timeT
Event::EventData::getNotationDuration() const
{
! if (!m_properties) return m_absoluteTime;
! PropertyMap::const_iterator i = m_properties->find(NotationDuration);
! if (i == m_properties->end()) return m_duration;
else return static_cast<PropertyStore<Int> *>(i->second)->getData();
}
void
Event::EventData::setTime(const PropertyName &name, timeT t, timeT deft)
{
! if (!m_properties) m_properties = new PropertyMap();
! PropertyMap::iterator i = m_properties->find(name);
if (t != deft) {
! if (i == m_properties->end()) {
! m_properties->insert(PropertyPair(name, new PropertyStore<Int>(t)));
} else {
static_cast<PropertyStore<Int> *>(i->second)->setData(t);
}
! } else if (i != m_properties->end()) {
delete i->second;
! m_properties->erase(i);
}
}
PropertyMap *
Event::find(const PropertyName &name, PropertyMap::iterator &i)
{
! PropertyMap *map = m_data->m_properties;
! if (!map || ((i = map->find(name)) == map->end())) {
map = m_nonPersistentProperties;
if (!map) return 0;
*************** Event::dump(ostream& out) const
*** 303,311 ****
<< "\n\tSub-ordering : " << m_data->m_subOrdering
<< "\n\tPersistent properties : \n";
! for (PropertyMap::const_iterator i = m_data->m_properties.begin();
! i != m_data->m_properties.end(); ++i) {
! out << "\t\t" << i->first << '\t' << *(i->second) << '\n';
}
if (m_nonPersistentProperties) {
--- 306,316 ----
<< "\n\tSub-ordering : " << m_data->m_subOrdering
<< "\n\tPersistent properties : \n";
! if (m_data->m_properties) {
! for (PropertyMap::const_iterator i = m_data->m_properties->begin();
! i != m_data->m_properties->end(); ++i) {
! out << "\t\t" << i->first << '\t' << *(i->second) << '\n';
! }
}
if (m_nonPersistentProperties) {
*************** Event::PropertyNames
*** 360,368 ****
Event::getPropertyNames() const
{
PropertyNames v;
! for (PropertyMap::const_iterator i = m_data->m_properties.begin();
! i != m_data->m_properties.end(); ++i) {
! v.push_back(i->first);
}
if (m_nonPersistentProperties) {
for (PropertyMap::const_iterator i = m_nonPersistentProperties->begin();
--- 365,375 ----
Event::getPropertyNames() const
{
PropertyNames v;
! if (m_data->m_properties) {
! for (PropertyMap::const_iterator i = m_data->m_properties->begin();
! i != m_data->m_properties->end(); ++i) {
! v.push_back(i->first);
! }
}
if (m_nonPersistentProperties) {
for (PropertyMap::const_iterator i = m_nonPersistentProperties->begin();
*************** Event::PropertyNames
*** 377,385 ****
Event::getPersistentPropertyNames() const
{
PropertyNames v;
! for (PropertyMap::const_iterator i = m_data->m_properties.begin();
! i != m_data->m_properties.end(); ++i) {
! v.push_back(i->first);
}
return v;
}
--- 384,394 ----
Event::getPersistentPropertyNames() const
{
PropertyNames v;
! if (m_data->m_properties) {
! for (PropertyMap::const_iterator i = m_data->m_properties->begin();
! i != m_data->m_properties->end(); ++i) {
! v.push_back(i->first);
! }
}
return v;
}
*************** size_t
*** 401,410 ****
Event::getStorageSize() const
{
size_t s = sizeof(Event) + sizeof(EventData) + m_data->m_type.size();
! for (PropertyMap::const_iterator i = m_data->m_properties.begin();
! i != m_data->m_properties.end(); ++i) {
! s += sizeof(i->first);
! s += i->second->getStorageSize();
}
if (m_nonPersistentProperties) {
for (PropertyMap::const_iterator i = m_nonPersistentProperties->begin();
--- 410,421 ----
Event::getStorageSize() const
{
size_t s = sizeof(Event) + sizeof(EventData) + m_data->m_type.size();
! if (m_data->m_properties) {
! for (PropertyMap::const_iterator i = m_data->m_properties->begin();
! i != m_data->m_properties->end(); ++i) {
! s += sizeof(i->first);
! s += i->second->getStorageSize();
! }
}
if (m_nonPersistentProperties) {
for (PropertyMap::const_iterator i = m_nonPersistentProperties->begin();
Index: base/Event.h
===================================================================
RCS file: /cvsroot/rosegarden/base/Event.h,v
retrieving revision 1.56
diff -u -p -c -r1.56 Event.h
*** base/Event.h 22 Apr 2003 15:14:30 -0000 1.56
--- base/Event.h 2 Jun 2003 09:51:13 -0000
*************** private:
*** 260,266 ****
timeT absoluteTime, timeT duration, short subOrdering);
EventData(const std::string &type,
timeT absoluteTime, timeT duration, short subOrdering,
! const PropertyMap &properties);
EventData *unshare();
~EventData();
unsigned int m_refCount;
--- 260,266 ----
timeT absoluteTime, timeT duration, short subOrdering);
EventData(const std::string &type,
timeT absoluteTime, timeT duration, short subOrdering,
! const PropertyMap *properties);
EventData *unshare();
~EventData();
unsigned int m_refCount;
*************** private:
*** 270,276 ****
timeT m_duration;
short m_subOrdering;
! PropertyMap m_properties;
// These are properties because we don't care so much about
// raw speed in get/set, but we do care about storage size for
--- 270,276 ----
timeT m_duration;
short m_subOrdering;
! PropertyMap *m_properties;
// These are properties because we don't care so much about
// raw speed in get/set, but we do care about storage size for
*************** private:
*** 317,322 ****
--- 317,323 ----
// returned iterator (in i) only valid if return map value is non-zero
PropertyMap *find(const PropertyName &name, PropertyMap::iterator &i);
+
const PropertyMap *find(const PropertyName &name,
PropertyMap::const_iterator &i) const {
PropertyMap::iterator j;
*************** private:
*** 326,335 ****
}
PropertyMap::iterator insert(const PropertyPair &pair, bool persistent) {
! PropertyMap *map =
! (persistent ? &m_data->m_properties : m_nonPersistentProperties);
! if (!map) map = m_nonPersistentProperties = new PropertyMap();
! return map->insert(pair).first;
}
#ifndef NDEBUG
--- 327,336 ----
}
PropertyMap::iterator insert(const PropertyPair &pair, bool persistent) {
! PropertyMap **map =
! (persistent ? &m_data->m_properties : &m_nonPersistentProperties);
! if (!*map) *map = new PropertyMap();
! return (*map)->insert(pair).first;
}
#ifndef NDEBUG
*************** Event::isPersistent(const PropertyName &
*** 419,425 ****
const PropertyMap *map = find(name, i);
if (map) {
! return (map == &m_data->m_properties);
} else {
throw NoData(name.getName(), __FILE__, __LINE__);
}
--- 420,426 ----
const PropertyMap *map = find(name, i);
if (map) {
! return (map == m_data->m_properties);
} else {
throw NoData(name.getName(), __FILE__, __LINE__);
}
*************** Event::set(const PropertyName &name, typ
*** 461,467 ****
PropertyMap *map = find(name, i);
if (map) {
! bool persistentBefore = (map == &m_data->m_properties);
if (persistentBefore != persistent) {
i = insert(*i, persistent);
map->erase(name);
--- 462,468 ----
PropertyMap *map = find(name, i);
if (map) {
! bool persistentBefore = (map == m_data->m_properties);
if (persistentBefore != persistent) {
i = insert(*i, persistent);
map->erase(name);
*************** Event::setMaybe(const PropertyName &name
*** 502,508 ****
PropertyMap *map = find(name, i);
if (map) {
! if (map == &m_data->m_properties) return; // persistent, so ignore it
PropertyStoreBase *sb = i->second;
--- 503,509 ----
PropertyMap *map = find(name, i);
if (map) {
! if (map == m_data->m_properties) return; // persistent, so ignore it
PropertyStoreBase *sb = i->second;
Index: base/test/test.C
===================================================================
RCS file: /cvsroot/rosegarden/base/test/test.C,v
retrieving revision 1.28
diff -u -p -c -r1.28 test.C
*** base/test/test.C 5 Feb 2003 19:59:47 -0000 1.28
--- base/test/test.C 2 Jun 2003 09:51:13 -0000
*************** int main(int argc, char **argv)
*** 240,254 ****
}
Event e1("note", 0);
! int gsCount = 20000;
st = times(&spare);
for (i = 0; i < gsCount; ++i) {
- #ifdef PROPERTY_NAME_IS_INT
- e1.set<Int>(i/4, i);
- #else
e1.set<Int>(names[i % NAME_COUNT], i);
- #endif
}
et = times(&spare);
cout << "Event: " << gsCount << " setInts: " << (et-st)*10 << "ms\n";
--- 240,250 ----
}
Event e1("note", 0);
! int gsCount = 200000;
st = times(&spare);
for (i = 0; i < gsCount; ++i) {
e1.set<Int>(names[i % NAME_COUNT], i);
}
et = times(&spare);
cout << "Event: " << gsCount << " setInts: " << (et-st)*10 << "ms\n";
*************** int main(int argc, char **argv)
*** 257,285 ****
j = 0;
for (i = 0; i < gsCount; ++i) {
if (i%4==0) sprintf(b+4, "%d", i);
- #ifdef PROPERTY_NAME_IS_INT
- e1.get<Int>(i/4);
- #else
j += e1.get<Int>(names[i % NAME_COUNT]);
- #endif
}
et = times(&spare);
cout << "Event: " << gsCount << " getInts: " << (et-st)*10 << "ms (result: "
<< j << ")\n";
st = times(&spare);
! for (i = 0; i < 100; ++i) {
Event e11(e1);
- #ifdef PROPERTY_NAME_IS_INT
- (void)e11.get<Int>(0);
- #else
(void)e11.get<Int>(names[i % NAME_COUNT]);
- #endif
}
et = times(&spare);
! cout << "Event: 100 copy ctors of " << e1.getStorageSize() << "-byte
element: "
<< (et-st)*10 << "ms\n";
! gsCount = 100000;
for (i = 0; i < NAME_COUNT; ++i) {
sprintf(b+4, "%ds", i);
--- 253,273 ----
j = 0;
for (i = 0; i < gsCount; ++i) {
if (i%4==0) sprintf(b+4, "%d", i);
j += e1.get<Int>(names[i % NAME_COUNT]);
}
et = times(&spare);
cout << "Event: " << gsCount << " getInts: " << (et-st)*10 << "ms (result: "
<< j << ")\n";
st = times(&spare);
! for (i = 0; i < 1000; ++i) {
Event e11(e1);
(void)e11.get<Int>(names[i % NAME_COUNT]);
}
et = times(&spare);
! cout << "Event: 1000 copy ctors of " << e1.getStorageSize() << "-byte
element: "
<< (et-st)*10 << "ms\n";
! // gsCount = 100000;
for (i = 0; i < NAME_COUNT; ++i) {
sprintf(b+4, "%ds", i);
*************** int main(int argc, char **argv)
*** 288,298 ****
st = times(&spare);
for (i = 0; i < gsCount; ++i) {
- #ifdef PROPERTY_NAME_IS_INT
- e1.set<String>(i/4 + 1000000, b);
- #else
e1.set<String>(names[i % NAME_COUNT], b);
- #endif
}
et = times(&spare);
cout << "Event: " << gsCount << " setStrings: " << (et-st)*10 << "ms\n";
--- 276,282 ----
*************** int main(int argc, char **argv)
*** 301,327 ****
j = 0;
for (i = 0; i < gsCount; ++i) {
if (i%4==0) sprintf(b+4, "%ds", i);
- #ifdef PROPERTY_NAME_IS_INT
- j += e1.get<String>(i/4 + 1000000).size();
- #else
j += e1.get<String>(names[i % NAME_COUNT]).size();
- #endif
}
et = times(&spare);
cout << "Event: " << gsCount << " getStrings: " << (et-st)*10 << "ms
(result: " << j << ")\n";
st = times(&spare);
! for (i = 0; i < 100; ++i) {
Event e11(e1);
- #ifdef PROPERTY_NAME_IS_INT
- (void)e11.get<String>(1000000);
- #else
(void)e11.get<String>(names[i % NAME_COUNT]);
- #endif
}
et = times(&spare);
! cout << "Event: 100 copy ctors of " << e1.getStorageSize() << "-byte
element: "
<< (et-st)*10 << "ms\n";
#else
cout << "Skipping test speed of Event\n";
--- 285,357 ----
j = 0;
for (i = 0; i < gsCount; ++i) {
if (i%4==0) sprintf(b+4, "%ds", i);
j += e1.get<String>(names[i % NAME_COUNT]).size();
}
et = times(&spare);
cout << "Event: " << gsCount << " getStrings: " << (et-st)*10 << "ms
(result: " << j << ")\n";
st = times(&spare);
! for (i = 0; i < 1000; ++i) {
Event e11(e1);
(void)e11.get<String>(names[i % NAME_COUNT]);
}
et = times(&spare);
! cout << "Event: 1000 copy ctors of " << e1.getStorageSize() << "-byte
element: "
<< (et-st)*10 << "ms\n";
+
+ st = times(&spare);
+ for (i = 0; i < 1000; ++i) {
+ Event e11(e1);
+ (void)e11.get<String>(names[i % NAME_COUNT]);
+ (void)e11.set<String>(names[i % NAME_COUNT], "blah");
+ }
+ et = times(&spare);
+ cout << "Event: 1000 copy ctors plus set<String> of " << e1.getStorageSize()
<< "-byte element: "
+ << (et-st)*10 << "ms\n";
+
+ // gsCount = 1000000;
+
+ st = times(&spare);
+ for (i = 0; i < gsCount; ++i) {
+ Event e21("dummy", i, 0, MIN_SUBORDERING);
+ }
+ et = times(&spare);
+ cout << "Event: " << gsCount << " event ctors alone: "
+ << (et-st)*10 << "ms\n";
+
+ st = times(&spare);
+ for (i = 0; i < gsCount; ++i) {
+ std::string s0("dummy");
+ std::string s1 = s0;
+ }
+ et = times(&spare);
+ cout << "Event: " << gsCount << " string ctors+assignents: "
+ << (et-st)*10 << "ms\n";
+
+ st = times(&spare);
+ for (i = 0; i < gsCount; ++i) {
+ Event e21("dummy", i, 0, MIN_SUBORDERING);
+ (void)e21.getAbsoluteTime();
+ (void)e21.getDuration();
+ (void)e21.getSubOrdering();
+ }
+ et = times(&spare);
+ cout << "Event: " << gsCount << " event ctors plus
getAbsTime/Duration/SubOrdering: "
+ << (et-st)*10 << "ms\n";
+
+ st = times(&spare);
+ for (i = 0; i < gsCount; ++i) {
+ Event e21("dummy", i, 0, MIN_SUBORDERING);
+ (void)e21.getAbsoluteTime();
+ (void)e21.getDuration();
+ (void)e21.getSubOrdering();
+ e21.set<Int>(names[0], 40);
+ (void)e21.get<Int>(names[0]);
+ }
+ et = times(&spare);
+ cout << "Event: " << gsCount << " event ctors plus one get/set and
getAbsTime/Duration/SubOrdering: "
+ << (et-st)*10 << "ms\n";
+
#else
cout << "Skipping test speed of Event\n";
