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

Remove raw() #1728

Closed
wants to merge 1 commit into from
Closed

Remove raw() #1728

wants to merge 1 commit into from

Conversation

gpotter2
Copy link
Member

@gpotter2 gpotter2 commented Dec 3, 2018

This PR is an old rebase

The raw function is supper inefficient, and can quite easily be avoided. It was a bad idea

This PR comes with an average improvement of dissection speed of 8%

TODO: fix last tests

@codecov-io
Copy link

codecov-io commented Dec 3, 2018

Codecov Report

Merging #1728 into master will decrease coverage by 7.43%.
The diff coverage is 74.03%.

@@            Coverage Diff             @@
##           master    #1728      +/-   ##
==========================================
- Coverage   85.59%   78.15%   -7.44%     
==========================================
  Files         181      129      -52     
  Lines       42133    30542   -11591     
==========================================
- Hits        36062    23870   -12192     
- Misses       6071     6672     +601
Impacted Files Coverage Δ
scapy/layers/tls/record_tls13.py 60% <0%> (-0.37%) ⬇️
scapy/layers/tls/automaton_cli.py 32.01% <0%> (-45.11%) ⬇️
scapy/scapypipes.py 83.98% <100%> (-1.37%) ⬇️
scapy/layers/radius.py 97.69% <100%> (ø) ⬆️
scapy/layers/tls/crypto/h_mac.py 80.59% <100%> (-11.47%) ⬇️
scapy/layers/sctp.py 96.68% <100%> (ø) ⬆️
scapy/layers/tls/record_sslv2.py 81.92% <100%> (-5.09%) ⬇️
scapy/modules/nmap.py 96.26% <100%> (-0.75%) ⬇️
scapy/layers/dns.py 91.51% <100%> (ø) ⬆️
scapy/modules/p0f.py 64.45% <100%> (-0.1%) ⬇️
... and 121 more

@p-l-
Copy link
Member

p-l- commented Dec 3, 2018

I don't get it. Why don't we just add raw = bytes to preserve the API?

@gpotter2
Copy link
Member Author

gpotter2 commented Dec 3, 2018

What was nice about raw is that it handled both bytes(IP()) and bytes(“test”, encoding=“utf8”) cases.

It’s a case by case :/

We can still set raw=bytes for people that used it on 2.4.0

@p-l-
Copy link
Member

p-l- commented Dec 3, 2018

I think it would be great if we kept raw() for people using it, and I'd rather continue documenting it instead of bytes(): I like the idea that it is not a type, so that if Python 4 changes again how raw bytes are manipulated, or if one day we decide to use another type, we won't have any problem with that.

@gpotter2 gpotter2 force-pushed the fast-rdpcap branch 3 times, most recently from f87a146 to 0398d1c Compare December 3, 2018 23:58
@guedou
Copy link
Member

guedou commented Dec 4, 2018

Can you explain the issue with the current raw() implementation?

@gpotter2
Copy link
Member Author

gpotter2 commented Dec 4, 2018

It’s slow: we do a try except on so many calls:/

If you cProfile.run a dissection of a PCAP, the most time-consuming function is the raw call.

@guedou
Copy link
Member

guedou commented Dec 4, 2018

I understand. What is the improvement compared to the script used in #642 ?

@gpotter2
Copy link
Member Author

gpotter2 commented Dec 7, 2018

This needs to be rethink. Dissection is about 8% faster but building is now slower.

@gpotter2 gpotter2 closed this Dec 7, 2018
@gpotter2 gpotter2 deleted the fast-rdpcap branch December 7, 2018 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants