-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Padraig O'Sullivan wrote:
> On 5/5/09, Jay Pipes <[email protected]> wrote:
> 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?
Excellent question, Padraig! That's precisely why we do these
regression tests :) I'm going to have Mats' bitvector class pushed to a
tree and benchmarked today, and we'll see if we see performance
improvements from it. If we do, then it will be up to the community to
decide the strategy going forward.
We could decide to revert entirely back to my_bitmap, to stick with
Mats' very STL-like class, or modify and submit a patch for the GCC's
bitset implementation, or just use the STL bitset as-is...
Don't be concerned that we run into these regressions. Part of the idea
of Drizzle is to identify these kind of cases and develop a strategy
that weighs maintainability with performance. :)
Cheers!
Jay
>> -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
iEYEARECAAYFAkoAbpEACgkQ2upbWsB4UtFjyQCfbtac7/VhasG9+0ru4pR04Peb
0uoAn1GEroHCx7U55fVBXjU5HRt0+4Zw
=JePL
-----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
