As has been mentioned elsewhere the most important thing is that
Drizzle has automated regression tests in place.
And this has already proved its worth, in finding this problem so
quickly. As you say, there is no real substitute for performance
testing!
<rant>
However there are some things that get my old C-head cooking, and that
is when inefficiencies are obvious ("how is it implemented" was kind
of rhetorical).
From the 2 examples below:
1) "(x & y) == x" makes 2 passes over the complete data set and
creates a temporary bitmap on the stack for the result of the &
operation.
bitmap_is_subset(x, y) on the other hand makes one pass of the data
and returns early in the FALSE case.
2) My estimate below (10 times faster) was not just a wild guess. It
is based on the fact that the C++ implementation works bit-for-bit,
and the C implementation works on 32 bits at a time.
The point is to simply watch out for this sort of thing because
sometimes C++ encourages it:
1) The C++ bitmap has no subsetof() method. The official recommended
way of doing this is to use "(x & y) == x".
2) The bit-wise C++ implementation ha_get_max_bit() is the only way to
do it without fiddling with the internal implementation.
</rant>
On the other hand, I totally agree with you that C can encourage other
bad habits like just hacking the internal implementation without
regard for API!
So this is not a matter of which is better C or C++. It is a matter of
being aware of the pitfalls of both languages and consciously working
against them.
On May 6, 2009, at 7:08 PM, Monty Taylor wrote:
Paul McCullagh wrote:
And we should watch out for things like this:
The MySQL version:
bitmap_is_subset(x, y)
The C++ "way":
(x & y) == x
It looks cute, but how is it implemented!?
Well... from reading the code above, I don't know any more about how
bitmap_is_subset is implemented than I do about how operator& is
implemented.
For me, which ever of the methods we choose to use above (I know
stewart
hates the operator overloading), is that we actually have an API and
stick to it. (and that the API isn't defined as "this object is
implemented as an array of bytes, so just grab the internal array
pointer and modify it" :) )
The thing is, there are simply great ways to optimize this
operation if
you write custom code to do it.
Another problem I see is that the bitmap implementation basically
encourages sub-optimal code. Take a look at the Drizzle
implementation
of ha_get_max_bit() (a function I use in PBXT) below. The Drizzle
version is maybe 100 times more understandable, but I would guess the
MySQL version is at least 10 times faster!
Most of the time that I've heard this assertion be made about things
in
the MySQL code being 10 times faster, they haven't actually held up to
benchmarking scrutiny. Benchmarking is good...
In general the thing that makes me suspicious of the C++ standard
lib is
that it is not necessarily written to be "database server grade"
(especially if they put the emphasis on readability instead of
speed).
I hear what you are saying, but it reminds me of the continual
temptation to get that extra little bit of juice out by breaking the
API
and directly messing with the internals of the object that you are
dealing with, which is what your MySQL code below looks like it's
doing.
(although I'll admit I haven't actually looked in to what exactly a
my_bitmap_map is or what its last_word_ptr and last_word_mask do - it
does seem to me of a "hey, here are pointers to my internal impl...")
The main problem here wasn't a sub-optimal STL bitset
implementation, it
was a semantic/usage problem in that the original bitmap used here was
runtime sized and was replaced with a compile-time size object.
By "database server grade" I mean _every_ instruction counts.
Totally agree... but before we claim instructions in either direction,
we should make sure to actually look at instructions produced rather
than guessing that the C++ one makes more instructions. (and I get
sort
of touchy about that as you can see, because people are quick to
jump on
the C++ is slow bandwagon, and statements like "I'm sure it's slower"
only fuel that fire)
The point isn't to argue that C++ is better or worse. (as fun as
language flame wars can be) Rather, I just want to reitterate:
1) Use consistent API (allows for impl to be improved in future)
2) Directly test any claims of speed one way or the other
Monty
---------------------------------------------------------------
static u_int ha_get_max_bit(MX_BITMAP *map)
{
#ifdef DRIZZLED
size_t i;
if (!map->any())
return 0;
for (i=0; i<map->size(); i++) {
if (map->test(i))
break;
}
return i+1;
#else
my_bitmap_map *data_ptr = map->bitmap;
my_bitmap_map *end_ptr = map->last_word_ptr;
my_bitmap_map b;
u_int cnt = map->n_bits;
for (; end_ptr >= data_ptr; end_ptr--) {
if ((b = *end_ptr)) {
my_bitmap_map mask;
if (end_ptr == map->last_word_ptr && map->last_word_mask)
mask = map->last_word_mask >> 1;
else
mask = 0x80000000;
while (!(b & mask)) {
b = b << 1;
/* Should not happen, but if it does, we hang! */
if (!b)
return map->n_bits;
cnt--;
}
return cnt;
}
if (end_ptr == map->last_word_ptr)
cnt = ((cnt-1) / 32) * 32;
else
cnt -= 32;
}
return 0;
#endif
}
On May 6, 2009, at 9:03 AM, Stewart Smith wrote:
So, there was a perf regression noticed as a result of replacing
my_bitmap with std::bitset.
This is why:
These code changes:
- MY_BITMAP *save_read_set, *save_write_set;
+ bitset<MAX_FIELDS> *save_read_set, *save_write_set;
means we stop doing things like this:
- bitmap_init(&column_bitmap, bitmap, head->s->fields, false);
and instead statically say "this bitset has MAX_FIELDS bits".
For the bitmap_init() call, the my_bitmap code only allocates enough
buffer space for the number of fields (in this case, grabbing the
field
count from the table share).
For the std::bitset, this allocates room for MAX_FIELDS, which is
4096.
i.e. a full 512 byte bitmap.
So for tables with around 4000 columns, both implementations are
likely
close in performance :)
However, for the common case (e.g. sysbench), it isn't. I prove with
benchmarks (same code, just changing MAX_FIELDS down to 64) and not
handwaving:
$ uname -a
Linux willster 2.6.30-020630rc2-generic #020630rc2 SMP Wed Apr 15
13:20:18 UTC 2009 x86_64 GNU/Linux
$ grep 'model name' /proc/cpuinfo
model name : Intel(R) Core(TM)2 Duo CPU T7250 @ 2.00GHz
model name : Intel(R) Core(TM)2 Duo CPU T7250 @ 2.00GHz
$ sysbench --test=oltp --drizzle-host=127.0.0.1 --drizzle-port=9306
--drizzle-db=test --drizzle-mysql=off --drizzle-user=root
--db-ps-mode=disable --db-driver=drizzle --drizzle-table-
engine=myisam
--max-time=60 --max-requests=2000 --oltp-read-only=on
--oltp-table-size=20000 --num-threads=8 run
MAX_FIELDS=4096 with std::bitset
read/write requests: 28000 (3016.51 per sec.)
read/write requests: 28000 (2804.91 per sec.)
read/write requests: 28000 (2824.13 per sec.)
read/write requests: 28000 (2830.43 per sec.)
MAX_FIELDS=64 with std::bitset
read/write requests: 28000 (3274.91 per sec.)
read/write requests: 28000 (2981.82 per sec.)
read/write requests: 28000 (2921.28 per sec.)
read/write requests: 28000 (2934.67 per sec.)
read/write requests: 28000 (3295.95 per sec.)
http://gcc.gnu.org/onlinedocs/libstdc++/manual/bitset.html
Is a good read - especially the completely useless suggestions for
workarounds. "just burn megabytes of memory", or "limit what your
program can accept" or (my favourite) "just use the compiler and
linker
at runtime". Some redeption with theorising that the last solution
is
that of a "raving lunatic".
The problem is that vector<bool> doesn't behave like a normal
vector
anymore. There have been recent journal articles which discuss the
problems (the ones by Herb Sutter in the May and July/August 1999
issues
of C++ Report cover it well). Future revisions of the ISO C++
Standard
will change the requirement for vector<bool> specialization. In
the
meantime, deque<bool> is recommended (although its behavior is
sane,
you probably will not get the space savings, but the allocation
scheme
is different than that of vector).
So it's going to be vector<bool> to the rescue.
I then want to bench it against Mats Kindal's magic code.
--
Stewart Smith
_______________________________________________
Mailing list: https://launchpad.net/~drizzle-discuss
Post to : [email protected]
Unsubscribe : https://launchpad.net/~drizzle-discuss
More help : https://help.launchpad.net/ListHelp
--
Paul McCullagh
PrimeBase Technologies
www.primebase.org
www.blobstreaming.org
pbxt.blogspot.com
_______________________________________________
Mailing list: https://launchpad.net/~drizzle-discuss
Post to : [email protected]
Unsubscribe : https://launchpad.net/~drizzle-discuss
More help : https://help.launchpad.net/ListHelp
--
Paul McCullagh
PrimeBase Technologies
www.primebase.org
www.blobstreaming.org
pbxt.blogspot.com
_______________________________________________
Mailing list: https://launchpad.net/~drizzle-discuss
Post to : [email protected]
Unsubscribe : https://launchpad.net/~drizzle-discuss
More help : https://help.launchpad.net/ListHelp
