Protocol module #669
Protocol module #669
Conversation
| def read_uint8(self): | ||
| result = protocol.read_uint8(self._data, offset=self._position) | ||
| self._position += 1 | ||
| return result |
|
I did "just move". What do you want to do after that? |
|
@methane I was under the impression that the final goal was to remove the wrapping classes and use calls to simple functions with builtin datatypes. That's why I moved the actual parsing code into their own functions, because eventually the wrapping classes would go away. From #297 (comment)
Phase 2 would be separating out the functions that orchestrate packet parsing (like _read_ok_packet, _read_rowdata_packet, etc). In this PR, I attempted to remove half of the work of MysqlPacket to parsing functions. The next phase would be to remove the other half (the position tracking inside packets) to functions that orchestrate the overall process of parsing packets. At that point MysqlPacket isn't doing anything and can be removed along with its companion classes. The next phase would be replacing MysqlPacket with a tuple. This means that _read_packet would be returning tuples instead of instances of classes.
At that point, I believe the goal set forth initially will have been fulfilled. A C extension can then be created to significantly accelerate packet parsing and the orchestrating functions can use either the C-extension or the pure-python module. |
|
@methane, any feedback on the above comment? |
OK,
I felt this was wrong step. It only doubles the function call (= performance), without any simplification.
I think next step should convert PacketWrapper classes into simple function. Maybe, FieldDescriptorPacket can be converted too. Breaking MySQLPacket class will be next step, but I haven't decided to do it or not. |
This is an intermediate step, so temporarily increasing function call wasn't a concern. It was done to show that no functionality was being broken by using the simple functions. It meant that any call to MySqlPacket.read_uint8 could be replaced with a call to protocol/read_uint8 as they were now equivalent.
I feel at this stage, performance is the wrong goal. We should be focusing on getting the implementation correct for an easy C extension, and then optimizing that implementation. The endgame is to prepare things for a C extension which is where performance will become a primary goal. I've already done some performance analysis on my design to show that it provides a performance benefit. My PRs in this branch try to achieve a cleaner implemenation of the same design that I was experiementing with in my parser2 branch. The intermediate steps are designed to be easy to review. It's the performance of the final step that matters, not the steps getting there.
Again, I was under the impression that removing MySQLPacket and the other wrapping classes was the whole point of this effort. I don't understand your approach very well. Could you outline your steps more completely? |
|
2018年6月6日(水) 23:30 Ryan Grout <notifications@github.com>:
That's why I moved the actual parsing code into their own functions, because eventually the wrapping classes would go away.
...
In this PR, I attempted to remove half of the work of MysqlPacket to parsing functions.
I felt this was wrong step. It only doubles the function call (=
performance), without any simplification.
This is an intermediate step, so temporarily increasing function call
wasn't a concern. It was done to show that no functionality was being
broken by using the simple functions. It meant that any call to
MySqlPacket.read_uint8 could be replaced with a call to protocol/read_uint8
as they were now equivalent.
I want defer degrading medium step as possible. I may make a release
before final step.
The next phase would be to remove the other half (the position
tracking inside packets) to functions that orchestrate the overall
process of parsing packets. At that point MysqlPacket isn't doing
anything and can be removed along with its companion classes.
I think next step should convert PacketWrapper classes into simple
function.
It can be achieved without touching MySQLPacket class.
It would be easy to review, and no performance penalty.
I feel at this stage, performance is the wrong goal. We should be focusing
on getting the implementation correct for an easy C extension, and then
optimizing that implementation. The endgame is to prepare things for a C
extension which is where performance will become a primary goal.
C extension is optional, and may be not used on PyPy. And I may make
release before C extension.
So pure Python performance is important.
Maybe, FieldDescriptorPacket can be converted too.
Breaking MySQLPacket class will be next step, but I haven't decided to do
it or not.
Again, I was under the impression that removing MySQLPacket and the other
wrapping classes was the whole point of this effort. I don't understand
your approach very well. Could you outline your steps more completely?
1. Reduce unnecessary wrapper classes.
2. Maybe, make a release, with MySQL 8 support.
3. Survey tools relating to this project. (E.g. mycli, aiomysql, maybe
more.)
4. Think what should be done.
--
INADA Naoki <songofacandy@gmail.com>
|
|
Isn't the point of this branch to isolate this work until it's ready to merge to master? Releases can still be made from master, independent of the state of this branch. |
|
I don't make decision yet.
But master branch is only for bugfix.
I'll implement any new feature in devel branch.
2018年6月7日(木) 14:54 Ryan Grout <notifications@github.com>:
… Isn't the point of this branch to isolate the work until it's ready to
merge to master? Releases can still be made from master, independently of
the state of this branch.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#669 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAMLqMZnKePmeDL53HTNTs9LMG9fw5W_ks5t6MAngaJpZM4T3FRM>
.
|
Move parsing functions to a protocol module.
There are three main types of parsing functions: packet-level, fixed length, and variable length. Packet-level functions operate on the whole packet and do not accept an offset parameter. Fixed-length functions parse a known amount of bytes and return the only the result. Variable-length functions parse a variable amount of data and return a tuple of (bytes read, result).