I believe this PR is now ready to be reviewed. The things that still fail are related to incompatible changes in the client itself, for example:

• Missing IAM methods -get_iam_policy(),set iam policy(),test_iam_permission()
• Missingcreate_subscription() parameters -dead_letter_policy,expiration_policy, etc.
• Missingclient_config parameter in publisher client constructor (is there an alternative mechanism?)

We need to figure out how to handle these and I will add theUPGRADING.md file to the docs after all these things have been clarified.

Since this PR is quite complex, every pair of eyes would be beneficial. 🙂

• @pradn Please check if ordering keys are affected in any way due to the lack ofclient_config options - they need "infinite" timeouts, right?
• @anguillanneuf Any feedback on samples would be great, as you're probably most familiar with them.
• @busunkim96 General feedback on the migration process in case I missed something.

Let's not rush and take the time to review this thoroughly, thanks!

I don't think I understand the benchmark result very well; are the two different throughput numbers forcps_publisher_task.py andcps_subscriber_task.py respectively?

Also, just had a thought that may be relevant: is the manual layer invoking the asynchronous client or the synchronous? I see a lot of task/future names and semantics, which made me ask. The asynchronous surface has had basically no optimization focus.

I did some publisher profiling withyappi using roughlythe code the benchmark framework uses and it seems that in the main thread a lot of time (~70%) is spent constructingPubsubMessage instances inside thepublisher.publish() method:

fromgoogle.pubsub_v1importtypesasgapic_types...# Create the Pub/Sub message object.message=gapic_types.PubsubMessage(data=data,ordering_key=ordering_key,attributes=attrs)

FWIW, profiling the current released version (i.e. themaster) branch did now show that piece of code to be problematic.

Is instantiating aPubsubMessage more expensive with the microgenerator and are there ways to construct it faster, e.g. by circumventing any checks/extra logic that might be redundant for this use case?

Yes, constructing messages is more expensive with the microgenerator because of its use ofproto-plus. Depending on whatdata,ordering_key, andattrs (actual params, not formal) are, proto-plus may be doing non-trivial work to marshal python structures to protobuf structures.

Can you send me the raw data in some way? I'd like to take a closer look at which part of construction is expensive.
There's a potential hack to get around this, but it's not really something we can recommend our users do.
It's possible to interact with 'vanilla' proto types in Python and then just shove them into proto-plus types before passing the result into a method call.

E.g.

vanilla_pb=gapic_types.PubsubMessage.pb(data=data,ordering_key=ordering_key,attributes=attrs)proto_plus_pb=gapic_types.PubsubMessage.wrap(vanilla_pb)

@software-dov

There's a potential hack to get around this, but it's not really something we can recommend our users do.

This actually improved things considerably - the benchmark showed a publisher throughput of ~57 MB/s, which is only ~11% worse than the current stable version. In addition, thePubsubMessage instance is constructed internally (users only pass in the message data and kwargs), meaning that we can actually use it.

Can you send me the raw data in some way?

The following is roughly how the messages look like in the benchmark and my local script for profiling:

data=b"A"*250ordering_key=""attrs= {'clientId':'0','sendTime':'1598046901869','sequenceNumber':'0'}...message=gapic_types.PubsubMessage(data=data,ordering_key=ordering_key,attributes=attrs)

(sequenceNumber monotonically increases)

If using the hack, instantiation is considerably faster, but there is still quite a lot of time spent inMarshal.to_proto() - if we can speed this up further, it would be great.

I'll also see if the same can be used to improve subscriber performance.

I don't think I understand the benchmark result very well; are the two different throughput numbers for cps_publisher_task.py and cps_subscriber_task.py respectively?

Correct. The benchmark framework spins up separate compute instances for publisher and subscriber and then measures how well they perform individually.

Also, just had a thought that may be relevant: is the manual layer invoking the asynchronous client or the synchronous? I see a lot of task/future names and semantics, which made me ask.

It's the synchronous client, i.e.google.pubsub_v1.services.publisher.client.PublisherClient. Those futures are subclasses ofgoogle.api_core.future.Future and are managed manually by the (hand-written) publisher client itself.

I have some good news - circumventing the wrapper classes around the raw protobuf messages to speed up instantiation and attribute access seems to help a lot. Running the benchmarks with the two most recent experimental commits produces the following:

INFO-Results for cps-gcloud-python-publisher:1157113 [main] INFO com.google.pubsub.flic.Driver  - Results for cps-gcloud-python-publisher:INFO-50%: 5.0625 - 7.593751157124 [main] INFO com.google.pubsub.flic.Driver  - 50%: 5.0625 - 7.59375INFO-99%: 86.49755859375 - 129.7463378906251157124 [main] INFO com.google.pubsub.flic.Driver  - 99%: 86.49755859375 - 129.746337890625INFO-99.9%: 194.6195068359375 - 291.929260253906251157124 [main] INFO com.google.pubsub.flic.Driver  - 99.9%: 194.6195068359375 - 291.92926025390625INFO-Average throughput: 57.02 MB/s1157126 [main] INFO com.google.pubsub.flic.Driver  - Average throughput: 57.02 MB/sINFO-Results for cps-gcloud-python-subscriber:1157126 [main] INFO com.google.pubsub.flic.Driver  - Results for cps-gcloud-python-subscriber:INFO-50%: 2216.8378200531006 - 3325.2567300796511157127 [main] INFO com.google.pubsub.flic.Driver  - 50%: 2216.8378200531006 - 3325.256730079651INFO-99%: 16834.112196028233 - 25251.168294042351157127 [main] INFO com.google.pubsub.flic.Driver  - 99%: 16834.112196028233 - 25251.16829404235INFO-99.9%: 25251.16829404235 - 37876.752441063521157128 [main] INFO com.google.pubsub.flic.Driver  - 99.9%: 25251.16829404235 - 37876.75244106352INFO-Average throughput: 57.83 MB/s1157128 [main] INFO com.google.pubsub.flic.Driver  - Average throughput: 57.83 MB/s

The performance hit is now only around 10% (publisher ~12%, subscriber ~8%), which might actually be acceptable.

Optimizingmarshal.to_proto is a mid-low priority ongoing effort. The good news is that any changes made to proto-plus will be available to all users for free and transparently, so if the new throughput numbers are acceptable they may eventually get better without having to cut a new release.
The not so good news is that I don't thinkto_proto is likely to get much faster in the general case. There may be specific instances we can optimize, or some crazy fast-path shenanigans, but the code there is doing nonzero work in order to facilitate a more flexible and ergonomic interface. The downside of proto-plus is that its user visible benefits come with a performance cost.

My bad, when I said "the raw data" above, I meant the profiling data. I don't particularly want to set up the benchmark environment, but I do want to try looking through the data to see what the hot spots are and if they could be optimized.

I have a special, optimized-but-not-reviewed-or-production-ready tarball of proto-plus. Is it possible to run the benchmark suite using it? My own benchmarking has these changes saving about 10-15 milliseconds out of about 500 milliseconds. YMMV, but if it improves the throughput enough it could be worth putting the proto-plus changes up for review.
proto-plus-1.7.1.tar.gz

I have a special, optimized-but-not-reviewed-or-production-ready tarball of proto-plus. Is it possible to run the benchmark suite using it?

Ifpip can access and install it, we can specify this version as a dependency in the framework'srequirements.txt, should be doable.

Just to clarify, would you like to run the benchmark using the tip of this PR branch, i.e. the version that already includes the optimizations that try to bypass the protobuf buffer classes? Or on the version that excludes this last commit and uses the wrapper classes more heavily?

My bad, when I said "the raw data" above, I meant the profiling data.

No worries, sent you an email with the data just now.

The tip of this PR branch.

Changes are also available in this branch of my fork:https://github.com/software-dov/proto-plus-python/tree/hackaday

First run, using proto-plus 1.7.1 on the tip of this PR branch. The results seem more or less similar to the previous benchmark:

INFO-Results for cps-gcloud-python-publisher:1151295 [main] INFO com.google.pubsub.flic.Driver  - Results for cps-gcloud-python-publisher:INFO-50%: 5.0625 - 7.593751151304 [main] INFO com.google.pubsub.flic.Driver  - 50%: 5.0625 - 7.59375INFO-99%: 86.49755859375 - 129.7463378906251151304 [main] INFO com.google.pubsub.flic.Driver  - 99%: 86.49755859375 - 129.746337890625INFO-99.9%: 194.6195068359375 - 291.929260253906251151304 [main] INFO com.google.pubsub.flic.Driver  - 99.9%: 194.6195068359375 - 291.92926025390625INFO-Average throughput: 56.58 MB/s1151306 [main] INFO com.google.pubsub.flic.Driver  - Average throughput: 56.58 MB/sINFO-Results for cps-gcloud-python-subscriber:1151306 [main] INFO com.google.pubsub.flic.Driver  - Results for cps-gcloud-python-subscriber:INFO-50%: 1477.8918800354004 - 2216.83782005310061151306 [main] INFO com.google.pubsub.flic.Driver  - 50%: 1477.8918800354004 - 2216.8378200531006INFO-99%: 25251.16829404235 - 37876.752441063521151307 [main] INFO com.google.pubsub.flic.Driver  - 99%: 25251.16829404235 - 37876.75244106352INFO-99.9%: 37876.75244106352 - 56815.1286615952851151307 [main] INFO com.google.pubsub.flic.Driver  - 99.9%: 37876.75244106352 - 56815.128661595285INFO-Average throughput: 57.37 MB/s1151307 [main] INFO com.google.pubsub.flic.Driver  - Average throughput: 57.37 MB/s

I also did another run, the measured throughput was around 55 MB/s - a bit worse, but within the normal results variance (the typical difference appears to be somewhere in the 1-2 MB/s range).

Okay. In light of those numbers I'm inclined not to merge the proto-plus changes. Is there an approval process for accepting the throughput regression?

Please just mind that that the proto-plus optimizations might not have a noticeable effect here, as the optimizations added here actively try to circumvent proto-plus, but other libraries might still see benefits.

I'm not aware of any formal processes, but we do have weekly PubSub meetings on Thursdays where we discuss these things. I added the question to the agenda whether a -10% performance hit is acceptable in the new major release.

Update: Was confirmed, -10% is good enough for now, although we should strive to improve this further in the mid-term.

@cguardia That's fine, appreciated. I will re-generate and benchmark the code again now, just in case, and see if we are ready for the major release.

After re-generating the code yet again, performance still appears to be in line with the previous benchmarks:

INFO-Results for cps-gcloud-python-publisher:1095535 [main] INFO com.google.pubsub.flic.Driver  - Results for cps-gcloud-python-publisher:INFO-50%: 5.0625 - 7.593751095545 [main] INFO com.google.pubsub.flic.Driver  - 50%: 5.0625 - 7.59375INFO-99%: 86.49755859375 - 129.7463378906251095546 [main] INFO com.google.pubsub.flic.Driver  - 99%: 86.49755859375 - 129.746337890625INFO-99.9%: 194.6195068359375 - 291.929260253906251095546 [main] INFO com.google.pubsub.flic.Driver  - 99.9%: 194.6195068359375 - 291.92926025390625INFO-Average throughput: 56.04 MB/s1095547 [main] INFO com.google.pubsub.flic.Driver  - Average throughput: 56.04 MB/sINFO-Results for cps-gcloud-python-subscriber:1095547 [main] INFO com.google.pubsub.flic.Driver  - Results for cps-gcloud-python-subscriber:INFO-50%: 194.6195068359375 - 291.929260253906251095549 [main] INFO com.google.pubsub.flic.Driver  - 50%: 194.6195068359375 - 291.92926025390625INFO-99%: 16834.112196028233 - 25251.168294042351095549 [main] INFO com.google.pubsub.flic.Driver  - 99%: 16834.112196028233 - 25251.16829404235INFO-99.9%: 37876.75244106352 - 56815.1286615952851095550 [main] INFO com.google.pubsub.flic.Driver  - 99.9%: 37876.75244106352 - 56815.128661595285INFO-Average throughput: 56.33 MB/s1095550 [main] INFO com.google.pubsub.flic.Driver  - Average throughput: 56.33 MB/s

Merging. 🎉

Edit: Ah,@kamalaboulhosn expressed a wish to review theUPGRADING guide, will wait with merging a bit more.