Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement BitFieldStruct feature, add test #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

waszil
Copy link

@waszil waszil commented Nov 20, 2019

I have ...

  • a question
  • found a bug
  • a change request

Hi, great package!
I have created a new class for handling bitfields as structs, so that they can be specified like in c. (unlike the current BitFields class with the string based syntax)
I created a test as well for the new feature.

@malinoff
Copy link
Owner

Hi, thanks for your PR.
What's your use case for implementhing this? What problem could not be solved using existing BitFields mechanism?
Also, this project raised mainly from construct library being incredibly slow even when not doing fancy stuff. Could you please benchmark how much your BitFieldStruct is slower than BitFields?

@waszil
Copy link
Author

waszil commented Nov 20, 2019

Hi, I also used construct, and I don't like their weird syntax using "/" in the declarations, that's why I found your project. With the BitFields, I have the same problem: it works, but I think it is more elegant to declare the bit fields the same ways as you can do other fields in the Struct class, then having to specify them as a string.
And answering your other question, it is also somewhat faster, as the BitFields class has to manipulate strings for parsing/building, and my solution uses integer arithmetic.
Here is a quick benchmark that shows the speed difference for a simple example:

class BFS(BitFieldStruct):
    foo = Bit(1)
    _ = BitPadding(6)
    bar = Bit(4)

bfs = BFS()
bf = BitFields('foo:1, padding:6, bar:4')

parse_this = b'\xff\xff'
build_this = {'foo': 1, 'bar': 15}
N = 100000

def _bf():
    bf.parse(parse_this)
    bf.build(build_this)

def _bfs():
    bfs.parse(parse_this)
    bfs.build(build_this)

%timeit _bf()
# 8.26 µs ± 21.8 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

%timeit _bfs()
# 6.59 µs ± 71.2 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

@malinoff
Copy link
Owner

Please, use a larger input byte array. The intention is never to parse just two bytes; also std.dev. of _bfs is amazingly high.

@waszil
Copy link
Author

waszil commented Nov 20, 2019

I don't see, why the input byte array size would matter, as the stream.read() will only read the appropriate number of bytes anyways. But I ran the benchmark with 1024 long input array. The standard deviation in the previous example was due to some background job by the os or antivirus... I ran it multiple times and took the average for mean and std.dev as well:
BitFields:
8.30875us +- 51ns
BitFieldStruct:
6.63125 +- 53ns

@YonatanRubin
Copy link

This feature seem very useful, but it will be nice if the BitFieldsStruct would be able to use normal constructs.

FinBird added a commit to FinBird/structures that referenced this pull request Jan 12, 2025
Implement BitField from malinoff#5
Implement Varint 
Add typehint
Move testcases to test_structures.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants