-
Notifications
You must be signed in to change notification settings - Fork 452
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
NEEDS VOLUNTEER: Communities ported to Protocol Buffers #2314
Conversation
Can one of the admins verify this patch? |
add to whitelist |
1 similar comment
add to whitelist |
Oh right, the testing framework does not have the Protocol Buffers python package installed. What would be the proper place to add this? |
@qstokkink |
bbq has protobuf now |
@whirm thanks! |
Let me know when you need the allchannel experiment to be enabled. I disabled it for now as it still hasn't the free cluster discovery stuff I made integrated. |
@whirm long answer: So, I think now would be a good time to see what kind of performance benefit would be gained from the new system. Or in the worst case: what kind of performance loss. Then it can be decided if this project is worth merging. |
Oh, apparently I didn't disable it for dispersy, you just need for the rest to pass for the experiment to be triggered. |
Nice progress on this PR. Thousands of lines of code altered... wow. |
c5e0f7d
to
295a4dc
Compare
Alright the results are in for the old protocol (with new interface): For reference, I will now specify the infrastructure changes between the old and the new system, as they exist right now. Old systemIncoming packets are identified by means of their metadata byte and then forwarded (conversion.py) to the appropriate handlers for their header information (authentication, distribution, etc.) and payload (payload.py). This requires community programmers to specify a conversion and a payload entry per message type. New systemThe new system expands on the old system's functionality by adding an additional message (the basemsg). This message is handled in the same fashion as specified by the old system (with a basemsg conversion and payload specification). However, this basemsg is handled as such by the BaseCommunity that it forwards its payload to other handlers. A side effect of this approach is that communities are no longer restricted to around 200 message definitions (instead the new limit is 2160 over all communities). Furthermore, messages no longer require their own conversion and payload definitions, instead this is handled by a Protocol Buffers .proto file. Note that another party trick is required for this to function as expected. This is because in the old system handlers for a particular message are statically specified with their network traversal information (authentication, destination, distribution and resolution) in a Message object. To handle this, a basemsg needs to be capable of using dynamic traversals. For the community programmer, this means that instead of initializing a Message, a traversal is registered with the same parameters (exluding handlers). Backward compatibilityBecause a basemsg is just another message, it can be defined alongside the old messages. This means that new communities (based on the BaseCommunity) can always receive old messages. This means that new communities can read from old communities, but will no longer write to old communities. However to avoid immediate isolation of nodes running old communities an implementation with a grace period has been made. This is done by having communities start by communicating using the old messages and switch to new messages once new messages have been detected. In other words, no-one will switch to the new messages unless someone switches to the new messages. |
bf9fe83
to
d198192
Compare
@devos50 How much of the communities are you already covering with the REST API tests? It seems you are already covering most (if not all) of the allchannel and channel communities. |
@qstokkink it's not that good in a sense that there are many error handling methods left uncovered. However, the core functionality of most communities (search, all channel, channel, tunnel) are covered with our (GUI) tests. I think we still need some stable and small unit tests to test the logic within these communities without being dependent on a running Tribler session but it's not really a top priority issue. |
@devos50 It's really hard (read: ugly private-field-dependant code) to separate the Session from a community. For instance the AllChannel community will either make a database stub or a real handler, internally, depending on whether or not it is initialized with a session. To make matters worse, these stubs are not shared between communities. Furthermore, because of the coupling side effects of the real handlers, checking proper database calls is only really possible using a Session's created databases. That said, using a very limited SessionStartupConfig it is possible to make some somewhat nimble unittests. What I did for my AllChannel unittests (local WIP, keep an eye out for it soon), is generation of the databases using a Session and then overwrite the stubs (private field access <- not cool) with the proper databases. This is the only way to get everything working with Dispersy's DebugNodes without getting Session singleton errors. Since your unittests are initialized and checked more elegantly than mine, I would have dropped my approach in favor of yours. However, since there is still some merit to it apparently, I will continue work on the low-level unittests. |
For anyone interested: the first results show an average 14% reduction in test completion time using Protocol Buffers. These results are based on the time it takes between the AllChannel community to create a message on one node and handle it on another node (ergo mostly serialization and unserialization of messages). |
dab00e8
to
cf0ed91
Compare
@whirm I created build instructions for (x64) Windows, which are not quite as elegant as I would have liked. I also see I triggered 5000 builds while wrestling with the ReStructuredText syntax. |
Copy **and rename** this file to: | ||
:code:`%FULLSOURCELOCATION%\python\protobuf.lib` | ||
|
||
Finally run :code:`python %FULLSOURCELOCATION%\python\setup.py install --cpp_implementation` **with a 32-bit python version**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We support both 32 and 64 bit builds of Tribler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@whirm The most recent version has the 64 bit instructions.
EDIT: In the even newer version I made the difference explicit.
Could someone with a Mac try the following for me? @devos50 are you available? Experimental/unverified Mac instructions:
Test:
|
@qstokkink no need to compile it by yourself :)
|
@devos50 thanks! That's much easier than the Windows build. |
1b34d7d
to
e896577
Compare
91a3589
to
ad7e76d
Compare
ad7e76d
to
5b9356c
Compare
@devos50 Any idea why all of my PR tests are being aborted by anonymous?
|
@qstokkink probably because if the spelling checker fails, it aborts the execution of the whole job. You should check if that's the case and if so, fix it so the failure gets ignored and the job continues to execute. |
@devos50 thanks, forgot a checkbox in GH_Tribler_PR :) |
8e92a98
to
50543d7
Compare
50543d7
to
9a8d18d
Compare
Well now.. I appear to be a bit of a ding dong. The reason the graph wasn't showing any records being received is because I didn't tell it to plot "basemsg" messages. 😕 Anyway, there are no known bugs remaining for this PR now (/again). EDIT: You can find all of the pretty graphs of both the old code and new code runs together here: |
@qstokkink can you explain this error? Should I reinstall protobuf on the Windows machines?
|
@qstokkink this breaks backwards compatibility right? If so, I would save this PR for Tribler 7.1 or maybe later. |
@devos50 (1) It has never been installed on Windows (I still need someone to verify the Windows install instructions). (2) In its current form the code IS backwards compatible (by means of additional code, which can be removed when backwards compatibility is broken); that said, I think this would be a good one to postpone after the 7.0 release, yes. |
Closing this in favor of performing a clean protobuf implementation on top of IPv8. Additional unit tests have already been exported to another PR (#2911). |
Related to #706, #2106.
Ported the AllChannel, Bartercast4, Channel, Demers and Template communities to Google's Protocol Buffers serialization. This implementation is backward compatible with the old wire format (± 2000 LOC will be thrown out when backward compatibility is removed).
Current status: