On 5/5/09, Jay Pipes <[email protected]> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Mats Kindahl wrote:
>> Jay Pipes wrote:
>>> Further narrowing down has focused on the revision series r982.1.1
>>> through r982.1.5. Attached is the diff for that, and somewhere in there
>>> is the regression.
>>>
>>> Note that the goal here is not necessarily to undo Padraig's work on
>>> standardizing Bitset, but to look into why/where specifically we're
>>> seeing that regression on concurrency loads >128. Remember, no shame,
>>> no blame, many hands make light work. :)
>>>
>>> Eyeballs please! :)
>>
>> OK, I have attached a some examples of pieces that look suspicious. They
>> are all
>> focused on the working (memory) set of a thread and where it is bigger
>> than it
>> is using the old bitmap structure.
>>
>> In summary:
>>
>> - Overallocation of memory in blocks of 512 bytes
>>
>> - Thrashing memory accesses forcing caches to be flushed and loaded very
>> often.
>>
>> - I think you would be better of using the attached implementation of a
>> dynamic
>> bitvector instead of bitset (I wrote it a few years ago), since that
>> will just
>> allocate the necessary memory.
>
> First of all, I'd like to say, thank you! As usual, your insight into
> these matters is most valuable :)
>
> I'm going to create a branch, use your bitvector class, and modify the
> original code and benchmark to see if we see a performance improvement.
> I'll update the list when I have results.
I'd also like to say thanks for your comments Mats, they are very
helpful. Looking at your dynamic bitvector implementation is certainly
a learning experience.
Hopefully, I can help with integrating this class soon Jay. I'm
currently swamped with exams and projects due to the end of semester
crunch so I won't be much help for the next 2 weeks.
I do have one question though. I was under the impression that the
point of tasks like this was to use STL standard classes (such as in
this case std::bitset). Mats bitvector class is pretty cool and if
performance is better with this class, I'd vote for using it. In
general though, I'm wondering if we come across a situation like this
where we see a regression due to something from the STL being used
instead of a custom class (probably unlikely), are we likely to stick
with the custom class or use a new custom class (such as Mats
btivector class) which addresses those issues?
-Padraig
>
> Thanks again, Mats. You rock.
>
> - -jay
>
>> Just my few cents,
>> Mats Kindahl
>>
>> ----------------------------
>> This looks strange, without having checked the context, I would look
>> closer at it.
>>
>> @@ -113,7 +113,7 @@
>> assert(bitmap);
>> if (result_field)
>> {
>> - bitmap->set(field->field_index);
>> + bitmap->set(result_field->field_index);
>> }
>> return 0;
>> }
>>
>>
>> ----------------------------
>> This allocates one temporary object of size 512 bytes (!) just to check if
>> any
>> bit is set. This will iterates over all 4096 fields, or all 512 bytes at
>> least
>> two times. If is_key_used() is mostly false, this will be a considerable
>> impact
>> on the execution time each time this function is called. Note that most
>> tables
>> do not have 4096 fields, but significantly less.
>>
>> -bool is_key_used(Table *table, uint32_t idx, const bitmap<MAX_FIELDS>
>> *fields)
>> +bool is_key_used(Table *table, uint32_t idx, const bitset<MAX_FIELDS>
>> *fields)
>> {
>> table->tmp_set.reset();
>> table->mark_columns_used_by_index_no_reset(idx, &table->tmp_set);
>> - /* TODO: change this to use std::bitset */
>> - if (bitmap_is_overlapping(&table->tmp_set, fields))
>> + /* Check if 2 bitsets are overlapping */
>> + bitset<MAX_FIELDS> tmp= *fields & table->tmp_set;
>> + if (tmp.any())
>> return 1;
>>
>> /*
>>
>>
>> ----------------------------
>> This will allocate two temporaries, each of size 512 bytes, and iterate
>> over
>> them to check if one bitset is a subset of the other.
>>
>> @@ -126,6 +127,19 @@
>> return static_cast<ha_rows>(x);
>> }
>>
>> +/*
>> + * This helper function returns true if map1 is a subset of
>> + * map2; otherwise it returns false.
>> + */
>> +static bool is_bitmap_subset(const bitset<MAX_FIELDS> *map1, const
>> bitset<MAX_FIELDS> *map2)
>> +{
>> + bitset<MAX_FIELDS> tmp1= *map2;
>> + tmp1.flip();
>> + bitset<MAX_FIELDS> tmp2= *map1 & tmp1;
>> + return (!tmp2.any());
>> +}
>> +
>> +
>> static int sel_cmp(Field *f,unsigned char *a,unsigned char *b,uint8_t
>> a_flag,uint8_t b_flag);
>>
>> static unsigned char is_null_string[2]= {1,0};
>>
>> ------------------------------
>> This will allocate two 512 bytes variables on the stack where the old
>> implementation just allocates memory for the columns actually used.
>>
>> @@ -698,8 +712,8 @@
>> bool quick; // Don't calulate possible keys
>>
>> uint32_t fields_bitmap_size;
>> - MY_BITMAP needed_fields; /* bitmask of fields needed by the query */
>> - MY_BITMAP tmp_covered_fields;
>> + bitset<MAX_FIELDS> needed_fields; /* bitmask of fields needed by the
>> query */
>> + bitset<MAX_FIELDS> tmp_covered_fields;
>>
>> key_map *needed_reg; /* ptr to SQL_SELECT::needed_reg */
>>
>>
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iEYEARECAAYFAkoAY58ACgkQ2upbWsB4UtFWogCcDUxkq9DP6tsf0+6l4vjoYPzQ
> W8cAniU9CzqjJTLcO1Gc4kHrLXF2I9Gs
> =0OGc
> -----END PGP SIGNATURE-----
>
_______________________________________________
Mailing list: https://launchpad.net/~drizzle-discuss
Post to : [email protected]
Unsubscribe : https://launchpad.net/~drizzle-discuss
More help : https://help.launchpad.net/ListHelp
