[GIL] About `packed_channel_value` (possible improvement) - Boost-users - lists.preview.boost.org

newer
Re: [Boost-users] Boost-users...

[GIL] About `packed_channel_value` (possible improvement)

older
[ASIO] Any HTML5 video streaming...

TONGARI

13 Feb 2013 13 Feb '13

4:48 p.m.

Hi, I took a look into "channel.hpp" and found that packed_channel_value is not very efficient in construction. There are: packed_channel_value(integer_t v) { _value = static_cast< integer_t >( v % num_values ); } // I have no idea why this exists, seems OK to remove it template <typename Scalar> packed_channel_value(Scalar v) { _value = static_cast< integer_t >( v ) % num_values; } IIUC, "v % num_values" above could be replaced with a much cheaper "v & low_bits_mask_t<NumBits>::sig_bits_fast". That is: packed_channel_value(integer_t v) : _value(v & low_bits_mask_t<NumBits>::sig_bits_fast) {} What do you think? Am I missing some point? Thanks.

Attachments:

attachment.html (text/html — 986 bytes)

Reply

Sign in to reply online Use email software

Show replies by date

Christian Henning

13 Feb 13 Feb

5:45 p.m.

Hi,

packed_channel_value(integer_t v) { _value = static_cast< integer_t >( v % num_values ); }

// I have no idea why this exists, seems OK to remove it template <typename Scalar> packed_channel_value(Scalar v) { _value = static_cast< integer_t >( v ) % num_values; }

I always thought we need that but some testing shows it's not needed. I'll remove it.

IIUC, "v % num_values" above could be replaced with a much cheaper "v & low_bits_mask_t<NumBits>::sig_bits_fast". That is:

packed_channel_value(integer_t v) : _value(v & low_bits_mask_t<NumBits>::sig_bits_fast) {}

What do you think? Am I missing some point?

I think, you're correct and I always wondered why we need num_values and num_value_t. I think it's obsolete and might have been created before low_bits_mask_t was available or the developers didn't knew about it. In any event I'll update the trunk later. First I wanna make sure the gil tests run through. Please let me know if you find any more redundancies. I'm happy to fix that. Thanks again, Christian

Reply

Sign in to reply online Use email software

Christian Henning

11:13 p.m.

Hi,

...
// I have no idea why this exists, seems OK to remove it template <typename Scalar> packed_channel_value(Scalar v) { _value = static_cast< integer_t >( v ) % num_values; }

I always thought we need that but some testing shows it's not needed. I'll remove it.

Turns out this function is needed. For instance in channel_algorithm.hpp[449]. I'll leave it in for now. Christian

Reply

Sign in to reply online Use email software

Christian Henning

14 Feb 14 Feb

4:10 p.m.

On Wed, Feb 13, 2013 at 6:13 PM, Christian Henning wrote:

Hi,

...
...
// I have no idea why this exists, seems OK to remove it template <typename Scalar> packed_channel_value(Scalar v) { _value = static_cast< integer_t >( v ) % num_values; }

I have uploaded the revised packed_channel_value class. Please have a look when you get a chance. Thanks, Christian

Reply

Sign in to reply online Use email software

TONGARI

4:29 p.m.

2013/2/15 Christian Henning

On Wed, Feb 13, 2013 at 6:13 PM, Christian Henning wrote:

...
Hi,

...
...
// I have no idea why this exists, seems OK to remove it template <typename Scalar> packed_channel_value(Scalar v) { _value

=

...
static_cast< integer_t >( v ) % num_values; }

I have uploaded the revised packed_channel_value class. Please have a look when you get a chance.

Ah, too bad, I just edited the file to get it work w/o that overload... Ok, I'll check it now. Thanks

Reply

Sign in to reply online Use email software

TONGARI

6 p.m.

Hi Christian, 2013/2/15 TONGARI

2013/2/15 Christian Henning

...
On Wed, Feb 13, 2013 at 6:13 PM, Christian Henning wrote:

...
Hi,

...
...
// I have no idea why this exists, seems OK to remove it template <typename Scalar> packed_channel_value(Scalar v) {

_value =

...
static_cast< integer_t >( v ) % num_values; }

I have uploaded the revised packed_channel_value class. Please have a look when you get a chance.

Ah, too bad, I just edited the file to get it work w/o that overload... Ok, I'll check it now.

Seems you haven't made any change to `channel.hpp`. Here's my patch for some possible improvement with care of not breaking the current API., I only tested the testcase `channel.cpp`. If you find any flaw or improper, please let me know. Thanks

Reply

Sign in to reply online Use email software

TONGARI

6:47 p.m.

2013/2/15 TONGARI

Hi Christian,

2013/2/15 TONGARI

...
2013/2/15 Christian Henning

...
On Wed, Feb 13, 2013 at 6:13 PM, Christian Henning wrote:

...
Hi,

...
...
// I have no idea why this exists, seems OK to remove it template <typename Scalar> packed_channel_value(Scalar v) {

_value =

...
static_cast< integer_t >( v ) % num_values; }

I have uploaded the revised packed_channel_value class. Please have a look when you get a chance.

Ah, too bad, I just edited the file to get it work w/o that overload... Ok, I'll check it now.

Seems you haven't made any change to `channel.hpp`. Here's my patch for some possible improvement with care of not breaking the current API., I only tested the testcase `channel.cpp`.

If you find any flaw or improper, please let me know.

Sorry, there's a fatal bug, please ignore the previous, here's the fixed one.

Reply

Sign in to reply online Use email software

Christian Henning

6:57 p.m.

Hi,

...
Seems you haven't made any change to `channel.hpp`. Here's my patch for some possible improvement with care of not breaking the current API., I only tested the testcase `channel.cpp`.

If you find any flaw or improper, please let me know.

Sorry, there's a fatal bug, please ignore the previous, here's the fixed one.

Could you send privately me your channel.hpp. For some reason I cannot apply the patch with my subversion. Thanks!

Reply

Sign in to reply online Use email software

4392

Age (days ago)

4393

Last active (days ago)

Download

7 comments

2 participants

tags

participants (2)

Christian Henning
TONGARI